[FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

[FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

[Stefano Sabatini]

x4->params.analyse.b_fast_pskip should only be forced in case mb_info
is set.

Fix output change introduced in 418c954e318.
---
 libavcodec/libx264.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 1a7dc7bdd5..a2877d7f75 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -1190,7 +1190,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
     }
 
     x4->params.analyse.b_mb_info = x4->mb_info;
-    x4->params.analyse.b_fast_pskip = 1;
+    if (x4->mb_info) {
+        x4->params.analyse.b_fast_pskip = x4->mb_info;
+    }
 
     // update AVCodecContext with x264 parameters
     avctx->has_b_frames = x4->params.i_bframe ?
-- 
2.25.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

[Carotti, Elias via ffmpeg-devel]

Hi

-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Stefano Sabatini
Sent: Friday, August 25, 2023 12:01 PM
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: Stefano Sabatini <stefasab@gmail.com>
Subject: [EXTERNAL] [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



x4->params.analyse.b_fast_pskip should only be forced in case mb_info
is set.

Fix output change introduced in 418c954e318.
---
 libavcodec/libx264.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index 1a7dc7bdd5..a2877d7f75 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -1190,7 +1190,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
     }

     x4->params.analyse.b_mb_info = x4->mb_info;
-    x4->params.analyse.b_fast_pskip = 1;
+    if (x4->mb_info) {
+        x4->params.analyse.b_fast_pskip = x4->mb_info;
+    }

     // update AVCodecContext with x264 parameters
     avctx->has_b_frames = x4->params.i_bframe ?
--
2.25.1


Sorry for the delay. I agree, this was missing in the patch.
Best
Elias

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".



NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

[Kieran Kunhya]

On Thu, 31 Aug 2023 at 15:12, Carotti, Elias via ffmpeg-devel <
ffmpeg-devel@ffmpeg.org> wrote:

> Hi
>
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Stefano
> Sabatini
> Sent: Friday, August 25, 2023 12:01 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Cc: Stefano Sabatini <stefasab@gmail.com>
> Subject: [EXTERNAL] [FFmpeg-devel] [PATCH] lavc/libx264: enable
> x4->params.analyse.b_fast_pskip if mb_info is set
>
> CAUTION: This email originated from outside of the organization. Do not
> click links or open attachments unless you can confirm the sender and know
> the content is safe.
>
>
>
> x4->params.analyse.b_fast_pskip should only be forced in case mb_info
> is set.
>
> Fix output change introduced in 418c954e318.
> ---
>  libavcodec/libx264.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index
> 1a7dc7bdd5..a2877d7f75 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -1190,7 +1190,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      }
>
>      x4->params.analyse.b_mb_info = x4->mb_info;
> -    x4->params.analyse.b_fast_pskip = 1;
> +    if (x4->mb_info) {
> +        x4->params.analyse.b_fast_pskip = x4->mb_info;
> +    }
>
>      // update AVCodecContext with x264 parameters
>      avctx->has_b_frames = x4->params.i_bframe ?
> --
> 2.25.1
>
>
> Sorry for the delay. I agree, this was missing in the patch.
> Best
> Elias
>

What does this patch actually do?

Kieran
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

[Kieran Kunhya]

On Thu, 31 Aug 2023 at 15:30, Kieran Kunhya <kierank@obe.tv> wrote:

>
>
> On Thu, 31 Aug 2023 at 15:12, Carotti, Elias via ffmpeg-devel <
> ffmpeg-devel@ffmpeg.org> wrote:
>
>> Hi
>>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Stefano Sabatini
>> Sent: Friday, August 25, 2023 12:01 PM
>> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
>> Cc: Stefano Sabatini <stefasab@gmail.com>
>> Subject: [EXTERNAL] [FFmpeg-devel] [PATCH] lavc/libx264: enable
>> x4->params.analyse.b_fast_pskip if mb_info is set
>>
>> CAUTION: This email originated from outside of the organization. Do not
>> click links or open attachments unless you can confirm the sender and know
>> the content is safe.
>>
>>
>>
>> x4->params.analyse.b_fast_pskip should only be forced in case mb_info
>> is set.
>>
>> Fix output change introduced in 418c954e318.
>> ---
>>  libavcodec/libx264.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index
>> 1a7dc7bdd5..a2877d7f75 100644
>> --- a/libavcodec/libx264.c
>> +++ b/libavcodec/libx264.c
>> @@ -1190,7 +1190,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>      }
>>
>>      x4->params.analyse.b_mb_info = x4->mb_info;
>> -    x4->params.analyse.b_fast_pskip = 1;
>> +    if (x4->mb_info) {
>> +        x4->params.analyse.b_fast_pskip = x4->mb_info;
>> +    }
>>
>>      // update AVCodecContext with x264 parameters
>>      avctx->has_b_frames = x4->params.i_bframe ?
>> --
>> 2.25.1
>>
>>
>> Sorry for the delay. I agree, this was missing in the patch.
>> Best
>> Elias
>>
>
> What does this patch actually do?
>
> Kieran
>

In particular why are you turning on fast_pskip silently based on a
completely different setting?

Kieran
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

[Stefano Sabatini]

On date Thursday 2023-08-31 15:38:09 +0100, Kieran Kunhya wrote:
> On Thu, 31 Aug 2023 at 15:30, Kieran Kunhya <kierank@obe.tv> wrote:
> 
> >
> >
> > On Thu, 31 Aug 2023 at 15:12, Carotti, Elias via ffmpeg-devel <
> > ffmpeg-devel@ffmpeg.org> wrote:
> >
> >> Hi
> >>
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Stefano Sabatini
> >> Sent: Friday, August 25, 2023 12:01 PM
> >> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> >> Cc: Stefano Sabatini <stefasab@gmail.com>
> >> Subject: [EXTERNAL] [FFmpeg-devel] [PATCH] lavc/libx264: enable
> >> x4->params.analyse.b_fast_pskip if mb_info is set
> >>
> >> CAUTION: This email originated from outside of the organization. Do not
> >> click links or open attachments unless you can confirm the sender and know
> >> the content is safe.
> >>
> >>
> >>
> >> x4->params.analyse.b_fast_pskip should only be forced in case mb_info
> >> is set.
> >>
> >> Fix output change introduced in 418c954e318.
> >> ---
> >>  libavcodec/libx264.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index
> >> 1a7dc7bdd5..a2877d7f75 100644
> >> --- a/libavcodec/libx264.c
> >> +++ b/libavcodec/libx264.c
> >> @@ -1190,7 +1190,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
> >>      }
> >>
> >>      x4->params.analyse.b_mb_info = x4->mb_info;
> >> -    x4->params.analyse.b_fast_pskip = 1;
> >> +    if (x4->mb_info) {
> >> +        x4->params.analyse.b_fast_pskip = x4->mb_info;
> >> +    }
> >>
> >>      // update AVCodecContext with x264 parameters
> >>      avctx->has_b_frames = x4->params.i_bframe ?
> >> --
> >> 2.25.1
> >>
> >>
> >> Sorry for the delay. I agree, this was missing in the patch.
> >> Best
> >> Elias
> >>
> >
> > What does this patch actually do?
> >
> > Kieran
> >
> 

> In particular why are you turning on fast_pskip silently based on a
> completely different setting?

The patch is fixing the regression introduced by the unconditional
setting of b_fast_pskip.

Now the question is if it makes sense to set mb_info without
b_fast_pskip (in both case this should be probably documented).

@Elias can you comment about the mb_info/b_fast_pskip use case?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

[Carotti, Elias via ffmpeg-devel]

On Thu, 2023-08-31 at 19:09 +0200, Stefano Sabatini wrote:
> 
<snip>
> 
> > In particular why are you turning on fast_pskip silently based on a
> > completely different setting?
> 
> The patch is fixing the regression introduced by the unconditional
> setting of b_fast_pskip.
> 
> Now the question is if it makes sense to set mb_info without
> b_fast_pskip (in both case this should be probably documented).
> 
> @Elias can you comment about the mb_info/b_fast_pskip use case?

Sorry again for the delay in responding. 
We can safely remove it altogether. It's true we don't need to set it
along with mb_info.
However, it doesn't do any harm, since fast_pskip is by default set to
true by libx264 and only turned off either explicitly by the user, or
when using the placebo preset, or when doing lossless encoding 
(constant QP == 0.)
So, I agree, let's remove these three lines.
Elias




NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

[Stefano Sabatini]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On date Saturday 2023-09-02 09:20:08 +0000, Carotti, Elias wrote:
> On Thu, 2023-08-31 at 19:09 +0200, Stefano Sabatini wrote:
> > 
> <snip>
> > 
> > > In particular why are you turning on fast_pskip silently based on a
> > > completely different setting?
> > 
> > The patch is fixing the regression introduced by the unconditional
> > setting of b_fast_pskip.
> > 
> > Now the question is if it makes sense to set mb_info without
> > b_fast_pskip (in both case this should be probably documented).
> > 
> > @Elias can you comment about the mb_info/b_fast_pskip use case?
> 
> Sorry again for the delay in responding. 
> We can safely remove it altogether. It's true we don't need to set it
> along with mb_info.
> However, it doesn't do any harm, since fast_pskip is by default set to
> true by libx264 and only turned off either explicitly by the user, or
> when using the placebo preset, or when doing lossless encoding 
> (constant QP == 0.)
> So, I agree, let's remove these three lines.

Thanks, updated.

[-- Attachment #2: 0001-lavc-libx264-do-not-unconditionally-set-x4-params.an.patch --]
[-- Type: text/x-diff, Size: 779 bytes --]

From ceb00a939ab2cd0fe146020b0e4e0d80b5d16a5d Mon Sep 17 00:00:00 2001
From: Stefano Sabatini <stefasab@gmail.com>
Date: Fri, 25 Aug 2023 11:35:01 +0200
Subject: [PATCH] lavc/libx264: do not unconditionally set
 x4->params.analyse.b_fast_pskip

Fix output change regression introduced in 418c954e318.
---
 libavcodec/libx264.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index ce849d6c9a..cc5e1ba5b1 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -1190,7 +1190,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
     }
 
     x4->params.analyse.b_mb_info = x4->mb_info;
-    x4->params.analyse.b_fast_pskip = 1;
 
     // update AVCodecContext with x264 parameters
     avctx->has_b_frames = x4->params.i_bframe ?
-- 
2.34.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

[Carotti, Elias via ffmpeg-devel]

-----Original Message-----
From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Stefano Sabatini
Sent: Saturday, September 2, 2023 5:45 PM
To: ffmpeg-devel@ffmpeg.org
Cc: kierank@obe.tv; Carotti, Elias <eliascrt@amazon.it>
Subject: RE: [EXTERNAL] [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



On date Saturday 2023-09-02 09:20:08 +0000, Carotti, Elias wrote:
> On Thu, 2023-08-31 at 19:09 +0200, Stefano Sabatini wrote:
> >
> <snip>
> >
> > > In particular why are you turning on fast_pskip silently based on 
> > > a completely different setting?
> >
> > The patch is fixing the regression introduced by the unconditional 
> > setting of b_fast_pskip.
> >
> > Now the question is if it makes sense to set mb_info without 
> > b_fast_pskip (in both case this should be probably documented).
> >
> > @Elias can you comment about the mb_info/b_fast_pskip use case?
>
> Sorry again for the delay in responding.
> We can safely remove it altogether. It's true we don't need to set it 
> along with mb_info.
> However, it doesn't do any harm, since fast_pskip is by default set to 
> true by libx264 and only turned off either explicitly by the user, or 
> when using the placebo preset, or when doing lossless encoding 
> (constant QP == 0.) So, I agree, let's remove these three lines.

Thanks, updated.

Ok for me
Elias



NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] lavc/libx264: enable x4->params.analyse.b_fast_pskip if mb_info is set

[Stefano Sabatini]

On date Tuesday 2023-09-05 12:43:35 +0000, ffmpeg-devel Mailing List wrote:
[...]
> On date Saturday 2023-09-02 09:20:08 +0000, Carotti, Elias wrote:
> > On Thu, 2023-08-31 at 19:09 +0200, Stefano Sabatini wrote:
> > >
> > <snip>
> > >
> > > > In particular why are you turning on fast_pskip silently based on 
> > > > a completely different setting?
> > >
> > > The patch is fixing the regression introduced by the unconditional 
> > > setting of b_fast_pskip.
> > >
> > > Now the question is if it makes sense to set mb_info without 
> > > b_fast_pskip (in both case this should be probably documented).
> > >
> > > @Elias can you comment about the mb_info/b_fast_pskip use case?
> >
> > Sorry again for the delay in responding.
> > We can safely remove it altogether. It's true we don't need to set it 
> > along with mb_info.
> > However, it doesn't do any harm, since fast_pskip is by default set to 
> > true by libx264 and only turned off either explicitly by the user, or 
> > when using the placebo preset, or when doing lossless encoding 
> > (constant QP == 0.) So, I agree, let's remove these three lines.
> >
> > Thanks, updated.
> 
> Ok for me

Applied, thanks.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".