Re: [FFmpeg-devel] [PATCH 2/3] avcodec: use the new event flags field instead of setting AVFrame.palette_has_changed

From: Anton Khirnov <anton@khirnov.net>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 2/3] avcodec: use the new event flags field instead of setting AVFrame.palette_has_changed
Date: Wed, 12 Apr 2023 14:39:24 +0200
Message-ID: <168130316429.9711.2495162257920531569@lain.khirnov.net> (raw)
In-Reply-To: <ee3b24f9-006d-449c-1c08-911886cc62e8@gmail.com>

Quoting James Almer (2023-04-12 13:59:11)
> On 4/12/2023 8:55 AM, Anton Khirnov wrote:
> > Quoting James Almer (2023-04-12 13:26:48)
> >> On 4/12/2023 5:09 AM, Anton Khirnov wrote:
> >>> Quoting James Almer (2023-04-11 22:38:50)
> >>>> diff --git a/doc/APIchanges b/doc/APIchanges
> >>>> index 59216231f9..85060768b3 100644
> >>>> --- a/doc/APIchanges
> >>>> +++ b/doc/APIchanges
> >>>> @@ -4,6 +4,7 @@ API changes, most recent first:
> >>>>    
> >>>>    2023-04-11 - xxxxxxxxxx - lavc 60.10.100 - avcodec.h
> >>>>      Add event_flags to AVCodecContext.
> >>>> +  Add AVCTX_EVENT_FLAG_PALETTE_HAS_CHANGED.
> >>>
> >>> I think we should first figure out how is the field supposed to be
> >>> useful to our users. Maybe it's not and we can just remove it.
> >>
> >> That's fine. I have no idea if it's actually used or useful (You can
> >> always just memcmp(prev->data[1], cur->data[1], 256) on every frame
> >> after all), but removing without replacement tends to be controversial,
> >> hence why i did this.
> > 
> > E.g. if the field is supposed to be useful in filters, then your change
> > would break that use case.
> 
> It's apparently not, as a grep shows hits only in libavcodec.
> > 
> > More generally I don't think we should be adding new APIs with no
> > understanding of how they are supposed to be used. Deprecating that
> > field entirely and waiting for someone to object seems better to me.
> 
> Event flags are a clear concept already done in lavf, so it's not being 
> done without knowing how they are supposed to be used. But ok, let's 
> just deprecate the field and see if anyone complains.

My concern is not about event flags in general (though I think those are
rather obscure and not that much used in lavf, so I'd be careful about
calling them 'clear'), but about specifically this palette change
information.

git log -S tells me it's never been used in our codebase for anything
since it was added in 2a2bbcb05fde (2003). So unless someone comes
forward with a use case I think just deprecating it is best.

-- 
Anton Khirnov
_______________________________________________
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".