[FFmpeg-devel] [PATCH 06/11] fftools/ffmpeg_filter: simplify choose_pix_fmts

[Niklas Haas]

On Fri, 12 Jan 2024 22:10:46 +0100 Michael Niedermayer <michael at niedermayer.cc> wrote:
> On Fri, Jan 12, 2024 at 09:26:03AM +0100, Niklas Haas wrote:
> > From: Niklas Haas <git at haasn.dev>
> > 
> > The only meaningful difference between choose_pix_fmts and the default
> > code was the inclusion of an extra branch for `keep_pix_fmt` being true.
> > 
> > However, in this case, we either:
> > 1. Force the specific `ofp->format` that we inherited from
> >    ofilter_bind_ost, or if no format was set:
> > 2. Print an empty format list
> > 
> > Both of these goals can be accomplished by simply moving the decision
> > logic to ofilter_bind_ost, to avoid setting any format list when
> > keep_pix_fmt is enabled. This is arguably cleaner as it moves format
> > selection logic to a single function. In the case of branch 1, nothing
> > else needs to be done as we already force the format provided in
> > ofp->format, if any is set. Add an assertion to verify this assumption
> > just in case.
> > 
> > (Side note: The "choose_*" family of functions are arguably misnomers,
> > as they should really be called "print_*" - their current behavior is to
> > print the relevant format lists to the `vf/af_format` filter arguments)
> > ---
> >  fftools/ffmpeg_filter.c | 49 ++++++++---------------------------------
> >  1 file changed, 9 insertions(+), 40 deletions(-)
> 
> 
> ./ffmpeg -y -i fate-suite/lena.pnm -pix_fmt +yuv444p -vf scale -strict -1 -bitexact -threads 2 -thread_type slice file-2s-444.jpg
> 
> Assertion !ost->keep_pix_fmt || (!ofp->format && !ofp->formats) failed at fftools/ffmpeg_filter.c:1240
> Aborted (core dumped)

Wrong logic on the assertion, should be:
  !ost->keep_pix_fmt || ofp->format != AV_PIX_FMT_NONE || !ofp->formats

The explanation here is that before this patch, keep_pix_fmt made the
filter disregard `ofp->formats` and always choose what was set in
`ofp->format` - even if it was NULL. This assertion guarantees that the
simplified choose_pix_fmts still does the same thing, since it will hit
either the ofp->format being set branch or the ofp->formats being NULL
branch.

Fixed locally.