[FFmpeg-devel] [PATCH v18 01/19] avcodec, avutil: Move enum AVSubtitleType

[Anton Khirnov]

Quoting Soft Works (2021-12-03 10:34:29)
> 
> 
> 
> At which place should FF_API_OLD_SUBTITLES get defined then (set to 1 for now)?

I see you figured that out sucessfully in the new version :)

> > > +enum AVSubtitleType {
> > > +
> > > +    /**
> > > +     * Subtitle format unknown.
> > > +     */
> > > +    AV_SUBTITLE_FMT_NONE = -1,
> > > +
> > > +    /**
> > > +     * Subtitle format unknown.
> > > +     */
> > > +    AV_SUBTITLE_FMT_UNKNOWN = 0,
> > > +    SUBTITLE_NONE = 0,          ///< Deprecated, use AV_SUBTITLE_FMT_NONE
> > instead.
> > 
> > This looks suspicious. Are you sure it's not going to break existing
> > code?
> 
> Yes I am. In the legacy API, SUBTITLE_NONE actually had a meaning like 
> "unspecified", "unknown" or "not set" (obviously - having a value of int 0).
> The ***_NONE values of the regular APIs though (e.g. AV_PIXFMT_NONE) 
> have a rather obscure semantic - sometimes being used as array sentinels, 
> sometimes for tristate sef/unset logic etc.

I don't think it's obscure and don't even see the difference --
AV_{PIX,SAMPLE}_FMT_NONE means exactly unspecified/unknown/not set. It's
the equivalent of the null pointer. And exactly like the null pointer,
it can be used as an array terminator.

> 
> SUBTITLE_NONE was nothing like that. It's the logical equivalent to 
> AV_SUBTITLE_FMT_UNKNOWN, despite the naming confusion.

"Nothing like that"? It sounds like exactly the same thing. Also note
that the doxygen for both in your patch says "subtitle format unknown".

> 
> I should also add that the actual define SUBTITLE_NONE has been used at a single
> place in all ffmpeg code only (https://github.com/FFmpeg/FFmpeg/search?q=SUBTITLE_NONE).

And as far as I can tell, it's used for empty subtitle blocks that do
not get output to the user. We do not need a special format for that.

> 
> The naming of the new constants is chosen to follow the API for audio and video
> (AV_SAMPLE_FMT_, AV_PIXFMT_). Actually I tried to establish equality to the
> other two wherever possible, even when something could have been "simplified",
> but I think that uniformity ("know one, know the other") is a much higher value
> from an API perspective than saving two or three functions and a few parameters 
> here and there.

Coherence with the other format APIs is a worthy goal, but seems to me
you could also achieve that by dropping AV_SUBTITLE_FMT_UNKNOWN
completely. What is it actually needed for?

> 
> 
> > > +    /**
> > > +     * Text Formatted as per ASS specification, contained
> > AVSubtitleRect.ass.
> > > +     */
> > > +    AV_SUBTITLE_FMT_ASS = 3,
> > > +    SUBTITLE_ASS = 3,           ///< Deprecated, use AV_SUBTITLE_FMT_ASS
> > instead.
> > > +
> > > +    AV_SUBTITLE_FMT_NB,
> > 
> > The use of this field should be documented. If it's a part of public
> > API, you are preventing any new types from being added without an
> > API/ABI break.
> 
> I guess you mean this text?
> 
>     AV_PIX_FMT_NB         ///< number of pixel formats, DO NOT USE THIS if you want to link with shared libav* because the number of formats might differ between versions

Yes -- if you add new values to the enum then the value of
AV_SUBTITLE_FMT_NB would increase, breaking binary compatibility with
any callers that used it.

-- 
Anton Khirnov