[FFmpeg-devel] [PATCH] avfilter/vf_scale: remove systematic PAL8 hack

[Michael Niedermayer]

Hi Niklas

On Fri, Dec 20, 2024 at 02:56:54PM +0100, Niklas Haas wrote:
> On Fri, 20 Dec 2024 08:37:40 -0500 "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
> > Hi,
> >
> > On Thu, Dec 19, 2024 at 5:19 PM Michael Niedermayer <michael at niedermayer.cc>
> > wrote:
> >
> > > Hi
> > >
> > > On Wed, Dec 18, 2024 at 12:48:03PM -0500, Ronald S. Bultje wrote:
> > > > Hi,
> > > >
> > > > On Wed, Dec 18, 2024 at 5:29 AM Niklas Haas <ffmpeg at haasn.xyz> wrote:
> > > >
> > > > > On Wed, 18 Dec 2024 02:32:07 +0100 Michael Niedermayer <
> > > > > michael at niedermayer.cc> wrote:
> > > > > > Hi
> > > > > >
> > > > > > On Tue, Dec 17, 2024 at 10:31:42AM +0100, Niklas Haas wrote:
> > > > > > > On Tue, 17 Dec 2024 01:48:02 +0100 Michael Niedermayer <
> > > > > michael at niedermayer.cc> wrote:
> > > > > > > > Hi
> > > > > > > >
> > > > > > > > On Mon, Dec 16, 2024 at 02:57:53PM +0100, Niklas Haas wrote:
> > > > > > > > > From: Niklas Haas <git at haasn.dev>
> > > > > > > > >
> > > > > > > > > This is not a good way of generating a PAL8 output.
> > > > > > > >
> > > > > > > > of course not
> > > > > > > >
> > > > > > > > but this breaks fate and features
> > > > > > >
> > > > > > > It doesn't really break a feature because we have a better
> > > replacement
> > > > > already
> > > > > > > included in libavfilter, IMO.
> > > > > >
> > > > > > swscale could convert to pal8 before, it cant after the patch.
> > > > > > So it lost a feature and breaks users code unless iam missing
> > > > > > something ?
> > > > >
> > > > > It is worth pointing out that *libswscale* does not directly output
> > > PAL8;
> > > > > this rather is (and always was) handled by vf_scale. So in some sense,
> > > this
> > > > > functionality already depends on libavfilter.
> > > > >
> > > > > That said, I do agree that simply regressing existing use cases should
> > > not
> > > > > be done without some sort of mechanism for automatically invoking the
> > > > > proper
> > > > > replacement.
> > > > >
> > > > > I will retract this patch for now, then, and put the corresponding
> > > issue on
> > > > > hold. I think that a full discussion of how to handle this better will
> > > have
> > > > > to wait until we have a better dither handling inside swscale.
> > > > >
> > > >
> > > > I would commit it, but instead of failing, emit a warning (recommending
> > > to
> > > > use the palettegen filter instead) and continue the old behaviour (as a
> > > > fallback).
> > >
> > > A future libswscale could include palettegen. (which would fix this issue
> > > instead of requiring users to switch to a different filter that may not
> > > even be available in the software they are using)
> > >
> >
> > To start, you agree that the current swscale hack to support some form of
> > palette generation is inferior to palettegen, right?
> >
> > Assuming you agree with that - which is hard to argue with - don't you
> > agree that for now, we should at the very least inform users that - if this
> > is what they're doing - what they're doing is inferior
> > (sws-palette-generation) and that there's a superior solution in-place
> > already (palettegen). For commandline users, this will be a string change
> > in their invocation. For API users, it's a bit more work but nothing major.
> > I can integrate libavfilter in my application in a few minutes. I have
> > posted sample code on stackoverflow doing that.
> >
> > For now, this is just an informative message (at loglevel=warning) telling
> > our users about this superior experience. At some point in the future (this
> > is probably 2 years from now?), the warning turns into an error. That
> > provides a clear timeline for this hypothetical swscale feature to be
> > implemented - or not. Both would be a great improvement for the vast
> > majority of our users who don't read their messages from their commandline
> > invocations until they fail.
> >
> > I only see positives here. And the best is: all of this already exists -
> > right there in FFmpeg, the toolkit which we all love. We only have to
> > inform our users about all this greatness. You must be excited about this,
> > no?
> 

> It would also be fairly straightforward to expose the palettegen functionality
> internally inside libavfilter and then add it directly to vf_scale. This would
> improve the status quo without anybody having to be the wiser.

yes


> 
> While it would be nice to have better palette-based dithering directly in
> libswscale as well, there is some risk of unnecessarily duplicating code at
> best, and reinventing a worse version of libavfilter at worst. Either way, that
> is something to consider *after* the ongoing rewrite of the core scaling
> functionality.

yes
I do think libswscale should do any image -> image convert (its kind of what its
intended to do)
if pal8 is left out, that seems like drawing the seperation at the wrong place
to me.
also the code from libavfilter can be moved into libswscale while libavfilter
can then depend on it and continue to present it to the user as it did before

> 
> I can't help but wonder if the better long-term solution here isn't to just
> improve the way libavfilter auto-inserts conversion filters, such that it could
> automatically insert a paletteuse filter when downstream requires PAL8.

PAL8 is tricky, encoders may benefit from similarities in palettes between frames
for others it might not matter. So automatic insertion may require some
"intelligence" to make the best choice

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241220/1c09287e/attachment.sig>