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

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Wednesday, December 1, 2021 2:35 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v18 01/19] avcodec, avutil: Move enum
> AVSubtitleType
> 
> Quoting Soft Works (2021-11-29 20:47:52)
> > Signed-off-by: softworkz <softworkz at hotmail.com>
> > ---
> >  libavcodec/avcodec.h | 19 +--------------
> >  libavutil/subfmt.h   | 58 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 59 insertions(+), 18 deletions(-)
> >  create mode 100644 libavutil/subfmt.h
> 
> This commit does way more than just moving the enum, it also
> deprecates existing enum values and adds new ones. That should be
> mentioned in the commit message and in doc/APIchanges.

Will do.

> Adding the header to the installed header list should also be moved to
> this commit.

Yes of course!


> The newly-deprecated enum values should be put under removal guards
> (i.e. #ifdef FF_API_OLD_SUBTITLES ...)

At which place should FF_API_OLD_SUBTITLES get defined then (set to 1 for now)?


> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 7ee8bc2b7c..b05c12e47e 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -35,6 +35,7 @@
> >  #include "libavutil/frame.h"
> >  #include "libavutil/log.h"
> >  #include "libavutil/pixfmt.h"
> > +#include "libavutil/subfmt.h"
> >  #include "libavutil/rational.h"
> >
> >  #include "codec.h"
> > @@ -2238,24 +2239,6 @@ typedef struct AVHWAccel {
> >   * @}
> >   */
> >
> > -enum AVSubtitleType {
> > -    SUBTITLE_NONE,
> > -
> > -    SUBTITLE_BITMAP,                ///< A bitmap, pict will be set
> > -
> > -    /**
> > -     * Plain text, the text field must be set by the decoder and is
> > -     * authoritative. ass and pict fields may contain approximations.
> > -     */
> > -    SUBTITLE_TEXT,
> > -
> > -    /**
> > -     * Formatted text, the ass field must be set by the decoder and is
> > -     * authoritative. pict and text fields may contain approximations.
> > -     */
> > -    SUBTITLE_ASS,
> > -};
> > -
> >  #define AV_SUBTITLE_FLAG_FORCED 0x00000001
> >
> >  typedef struct AVSubtitleRect {
> > diff --git a/libavutil/subfmt.h b/libavutil/subfmt.h
> > new file mode 100644
> > index 0000000000..5fc41d0ef2
> > --- /dev/null
> > +++ b/libavutil/subfmt.h
> > @@ -0,0 +1,58 @@
> > +/*
> > + * Copyright (c) 2021 softworkz
> > + *
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-
> 1301 USA
> > + */
> > +
> > +#ifndef AVUTIL_SUBFMT_H
> > +#define AVUTIL_SUBFMT_H
> > +
> > +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.

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

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).

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.


> > +    /**
> > +     * 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


Thank you very much,
softworkz