[FFmpeg-devel] [PATCH] all: Don't set AVClass.item_name to its default value

[Kacper Michajlow]

On Mon, 25 Dec 2023 at 11:48, Zhao Zhili <quinkblack at foxmail.com> wrote:
>
>
>
> > On Dec 25, 2023, at 18:21, Anton Khirnov <anton at khirnov.net> wrote:
> >
> > Quoting Zhao Zhili (2023-12-25 10:27:59)
> >>
> >>
> >>> On Dec 25, 2023, at 16:38, Anton Khirnov <anton at khirnov.net> wrote:
> >>>
> >>> Quoting Kacper Michajlow (2023-12-24 11:41:52)
> >>>> On Fri, 22 Dec 2023 at 14:57, Anton Khirnov <anton at khirnov.net> wrote:
> >>>>>
> >>>>> Quoting Andreas Rheinhardt (2023-12-22 14:48:45)
> >>>>>> Avoids relocations.
> >>>>>>
> >>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> >>>>>> ---
> >>>>>
> >>>>> Maybe mention that it's not needed after
> >>>>> acf63d5350adeae551d412db699f8ca03f7e76b9.
> >>>>
> >>>> This is not the only user of this API, no?
> >>>>
> >>>> I have a question for my own curiosity. This is ABI (and API) breaking
> >>>> change,
> >>>
> >>> It is not. This item was not guaranteed to be set, which was actually
> >>> the reason I wrote the patch that this one refers to.

It has never been marked as optional and while one can argue that it
was not marked as required either this ambiguity would be interpreted
that item_name is available by most. Moreover this is further
solidified by the first-party tools that made this assumption for more
than a decade.

I don't want to argue about technicalities and semantics, or whether
it is (was) required field. My point is that it is breaking change
that affects 3rd-party code and 1st-party code too.

You fixed log.c, but the exact same issue can be found in ffprobe.c
today. See https://github.com/FFmpeg/FFmpeg/blob/4fee63b241e0dd254436bf38df8b0635b2b666d8/fftools/ffprobe.c#L371
(and L385)

And by code search on github I could find more projects that are
affected. Some have directly copied previous version of the callback
from log.c.

> >> There is no problem to relax a restriction inside libavutil. However, since there is
> >> no explicit documentation on whether item_name can be null or not, user may make
> >> incorrect assumptions and depend on item_name not being null. I don’t think break
> >> user’s code suddenly is a good idea, although we can say it’s break since the beginning.
> >
> > My point is that the restriction never existed.
>
> I’m not argue against this.
>
> > There were always
> > AVClass instances that did not set that pointer. There were fewer of
> > them, but they did exist.
>
> acf63d5350adeae551d412db699f8ca03f7e76b9 just shows that even libavutil can make
> that incorrect assumption, we cannot expect more from users. If user’s code don’t trigger
> those instances with null pointer, it works from user’s viewpoint. It’s hard to believe that
> the bug found in MPV doesn’t exist in other  projects. We need another method to make
> such change smoothly.

Thanks, I couldn't summarize this better.

> >
> > --
> > Anton Khirnov
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org <mailto:ffmpeg-devel at ffmpeg.org>
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request at ffmpeg.org <mailto:ffmpeg-devel-request at ffmpeg.org> with subject "unsubscribe".
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".