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

[Anton Khirnov]

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.

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

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

> 
> 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?

> +
> +    /**
> +     * Bitmap area in AVSubtitleRect.data, pixfmt AV_PIX_FMT_PAL8.
> +     */
> +    AV_SUBTITLE_FMT_BITMAP = 1,
> +    SUBTITLE_BITMAP = 1,        ///< Deprecated, use AV_SUBTITLE_FMT_BITMAP instead.
> +
> +    /**
> +     * Plain text in AVSubtitleRect.text.
> +     */
> +    AV_SUBTITLE_FMT_TEXT = 2,
> +    SUBTITLE_TEXT = 2,          ///< Deprecated, use AV_SUBTITLE_FMT_TEXT instead.
> +
> +    /**
> +     * 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.

-- 
Anton Khirnov