[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 at obe.tv> wrote:
> 
> >
> >
> > On Thu, 31 Aug 2023 at 15:12, Carotti, Elias via ffmpeg-devel <
> > ffmpeg-devel at ffmpeg.org> wrote:
> >
> >> Hi
> >>
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> >> Stefano Sabatini
> >> Sent: Friday, August 25, 2023 12:01 PM
> >> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> >> Cc: Stefano Sabatini <stefasab at 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?