Re: [FFmpeg-devel] [PATCH 02/11] avcodec/raw: Reduce number of avpriv symbols

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Andreas Rheinhardt:
> libavcodec currently exports four avpriv symbols that deal with
> PixelFormatTags: avpriv_get_raw_pix_fmt_tags, avpriv_find_pix_fmt,
> avpriv_pix_fmt_bps_avi and avpriv_pix_fmt_bps_mov. The latter two are
> lists of PixelFormatTags, the former returns such a list and the second
> searches a list for a pixel format that matches a given fourcc; only
> one of the aforementioned three lists is ever searched.
> 
> Yet for avpriv_pix_fmt_bps_avi, avpriv_pix_fmt_bps_mov and
> avpriv_find_pix_fmt the overhead of exporting these functions actually
> exceeds the size of said objects (at least for ELF; the following numbers
> are for x64 Ubuntu 20.10):
> The code size of avpriv_find_pix_fmt is small (GCC 10.2 37B, Clang 11 41B),
> yet exporting it adds a 20B string for the name alone to the exporting
> as well as to each importing library; there is more: Four bytes in the
> exporting libraries .gnu.hash; two bytes each for the exporting as well
> as each importing libraries .gnu.version; 24B in the exporting as well
> as each importing libraries .dynsym; 16B+24B for an entry in .plt as
> well as the accompanying relocation entry in .rela.plt for each
> importing library.
> 
> The overhead for the lists is similar: The strings are 23B and the
> .plt+.rela.plt pair is replaced by 8B+24B for an entry in .got and
> a relocation entry in .rela.dyn. These lists have a size of 80 resp.
> 72 bytes.
> 
> Yet for ff_raw_pix_fmt_tags, exporting it is advantageous compared to
> duplicating it into libavformat and potentially libavdevice. Therefore
> this commit replaces all library uses of the four symbols with a single
> function that is exported for shared builds. It has an enum parameter
> to choose the desired list besides the parameter for the fourcc. New
> lists can be supported with new enum values.
> 
> Unfortunately, avpriv_get_raw_pix_fmt_tags could not be removed, as the
> fourcc2pixfmt tool uses the table of raw pix fmts. No other user of this
> function remains.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> 1. It would make sense to deduplicate avpriv_pix_fmt_bps_(mov|avi)
> size-wise if it is preferred to keep the lists accessible to users.
> 2. One could handle the fourcc2pixfmt case in avpriv_pix_fmt_find(),
> too, if one added another parameter (say count). In this case
> avpriv_pix_fmt_find() will return the first pixfmt with the desired
> fourcc if count is zero, the second one is count is 1 etc. (and
> AV_PIX_FMT_NONE in case there is none any more). This would also make
> this function more future-proof.
> 
>  libavcodec/raw.c     | 40 +++++++++++++++++++++++++++++++++++-----
>  libavcodec/raw.h     | 13 +++++++------
>  libavcodec/rawdec.c  |  8 ++++----
>  libavcodec/utils.c   | 11 -----------
>  libavdevice/dshow.c  |  2 +-
>  libavformat/avienc.c |  2 +-
>  libavformat/demux.c  |  2 +-
>  libavformat/movenc.c |  2 +-
>  8 files changed, 50 insertions(+), 30 deletions(-)
> 
> diff --git a/libavcodec/raw.c b/libavcodec/raw.c
> index 079d5c5d10..5efc1eb465 100644
> --- a/libavcodec/raw.c
> +++ b/libavcodec/raw.c
> @@ -28,7 +28,7 @@
>  #include "raw.h"
>  #include "libavutil/common.h"
>  
> -const PixelFormatTag ff_raw_pix_fmt_tags[] = {
> +static const PixelFormatTag raw_pix_fmt_tags[] = {
>      { AV_PIX_FMT_YUV420P, MKTAG('I', '4', '2', '0') }, /* Planar formats */
>      { AV_PIX_FMT_YUV420P, MKTAG('I', 'Y', 'U', 'V') },
>      { AV_PIX_FMT_YUV420P, MKTAG('y', 'v', '1', '2') },
> @@ -299,12 +299,12 @@ const PixelFormatTag ff_raw_pix_fmt_tags[] = {
>  
>  const struct PixelFormatTag *avpriv_get_raw_pix_fmt_tags(void)
>  {
> -    return ff_raw_pix_fmt_tags;
> +    return raw_pix_fmt_tags;
>  }
>  
>  unsigned int avcodec_pix_fmt_to_codec_tag(enum AVPixelFormat fmt)
>  {
> -    const PixelFormatTag *tags = ff_raw_pix_fmt_tags;
> +    const PixelFormatTag *tags = raw_pix_fmt_tags;
>      while (tags->pix_fmt >= 0) {
>          if (tags->pix_fmt == fmt)
>              return tags->fourcc;
> @@ -313,7 +313,7 @@ unsigned int avcodec_pix_fmt_to_codec_tag(enum AVPixelFormat fmt)
>      return 0;
>  }
>  
> -const PixelFormatTag avpriv_pix_fmt_bps_avi[] = {
> +static const PixelFormatTag pix_fmt_bps_avi[] = {
>      { AV_PIX_FMT_PAL8,    1 },
>      { AV_PIX_FMT_PAL8,    2 },
>      { AV_PIX_FMT_PAL8,    4 },
> @@ -326,7 +326,7 @@ const PixelFormatTag avpriv_pix_fmt_bps_avi[] = {
>      { AV_PIX_FMT_NONE,    0 },
>  };
>  
> -const PixelFormatTag avpriv_pix_fmt_bps_mov[] = {
> +static const PixelFormatTag pix_fmt_bps_mov[] = {
>      { AV_PIX_FMT_PAL8,      1 },
>      { AV_PIX_FMT_PAL8,      2 },
>      { AV_PIX_FMT_PAL8,      4 },
> @@ -337,3 +337,33 @@ const PixelFormatTag avpriv_pix_fmt_bps_mov[] = {
>      { AV_PIX_FMT_PAL8,     33 },
>      { AV_PIX_FMT_NONE,      0 },
>  };
> +
> +static enum AVPixelFormat find_pix_fmt(const PixelFormatTag *tags,
> +                                       unsigned int fourcc)
> +{
> +    while (tags->pix_fmt != AV_PIX_FMT_NONE) {
> +        if (tags->fourcc == fourcc)
> +            return tags->pix_fmt;
> +        tags++;
> +    }
> +    return AV_PIX_FMT_NONE;
> +}
> +
> +enum AVPixelFormat avpriv_pix_fmt_find(enum PixelFormatTagLists list,
> +                                       unsigned fourcc)
> +{
> +    const PixelFormatTag *tags;
> +
> +    switch (list) {
> +    case PIX_FMT_LIST_RAW:
> +        tags = raw_pix_fmt_tags;
> +        break;
> +    case PIX_FMT_LIST_AVI:
> +        tags = pix_fmt_bps_avi;
> +        break;
> +    case PIX_FMT_LIST_MOV:
> +        tags = pix_fmt_bps_mov;
> +        break;
> +    }
> +    return find_pix_fmt(tags, fourcc);
> +}
> diff --git a/libavcodec/raw.h b/libavcodec/raw.h
> index 6a041690b1..9a4ddef8fc 100644
> --- a/libavcodec/raw.h
> +++ b/libavcodec/raw.h
> @@ -28,20 +28,21 @@
>  #define AVCODEC_RAW_H
>  
>  #include "libavutil/pixfmt.h"
> -#include "internal.h"
>  
>  typedef struct PixelFormatTag {
>      enum AVPixelFormat pix_fmt;
>      unsigned int fourcc;
>  } PixelFormatTag;
>  
> -extern const PixelFormatTag ff_raw_pix_fmt_tags[]; // exposed through avpriv_get_raw_pix_fmt_tags()
> -
>  const struct PixelFormatTag *avpriv_get_raw_pix_fmt_tags(void);
>  
> -enum AVPixelFormat avpriv_find_pix_fmt(const PixelFormatTag *tags, unsigned int fourcc);
> +enum PixelFormatTagLists {
> +    PIX_FMT_LIST_RAW,
> +    PIX_FMT_LIST_AVI,
> +    PIX_FMT_LIST_MOV,
> +};
>  
> -extern av_export_avcodec const PixelFormatTag avpriv_pix_fmt_bps_avi[];
> -extern av_export_avcodec const PixelFormatTag avpriv_pix_fmt_bps_mov[];
> +enum AVPixelFormat avpriv_pix_fmt_find(enum PixelFormatTagLists list,
> +                                       unsigned fourcc);
>  
>  #endif /* AVCODEC_RAW_H */
> diff --git a/libavcodec/rawdec.c b/libavcodec/rawdec.c
> index b22e36e984..9724cce13f 100644
> --- a/libavcodec/rawdec.c
> +++ b/libavcodec/rawdec.c
> @@ -76,15 +76,15 @@ static av_cold int raw_init_decoder(AVCodecContext *avctx)
>  
>      if (   avctx->codec_tag == MKTAG('r','a','w',' ')
>          || avctx->codec_tag == MKTAG('N','O','1','6'))
> -        avctx->pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_mov,
> +        avctx->pix_fmt = avpriv_pix_fmt_find(PIX_FMT_LIST_MOV,
>                                        avctx->bits_per_coded_sample);
>      else if (avctx->codec_tag == MKTAG('W', 'R', 'A', 'W'))
> -        avctx->pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
> +        avctx->pix_fmt = avpriv_pix_fmt_find(PIX_FMT_LIST_AVI,
>                                        avctx->bits_per_coded_sample);
>      else if (avctx->codec_tag && (avctx->codec_tag & 0xFFFFFF) != MKTAG('B','I','T', 0))
> -        avctx->pix_fmt = avpriv_find_pix_fmt(ff_raw_pix_fmt_tags, avctx->codec_tag);
> +        avctx->pix_fmt = avpriv_pix_fmt_find(PIX_FMT_LIST_RAW, avctx->codec_tag);
>      else if (avctx->pix_fmt == AV_PIX_FMT_NONE && avctx->bits_per_coded_sample)
> -        avctx->pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
> +        avctx->pix_fmt = avpriv_pix_fmt_find(PIX_FMT_LIST_AVI,
>                                        avctx->bits_per_coded_sample);
>  
>      desc = av_pix_fmt_desc_get(avctx->pix_fmt);
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index a91a54b0dc..4d236ff1cd 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -436,17 +436,6 @@ void ff_color_frame(AVFrame *frame, const int c[4])
>      }
>  }
>  
> -enum AVPixelFormat avpriv_find_pix_fmt(const PixelFormatTag *tags,
> -                                       unsigned int fourcc)
> -{
> -    while (tags->pix_fmt >= 0) {
> -        if (tags->fourcc == fourcc)
> -            return tags->pix_fmt;
> -        tags++;
> -    }
> -    return AV_PIX_FMT_NONE;
> -}
> -
>  int avpriv_codec_get_cap_skip_frame_fill_param(const AVCodec *codec){
>      return !!(codec->caps_internal & FF_CODEC_CAP_SKIP_FRAME_FILL_PARAM);
>  }
> diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
> index ef78781865..ec1501ef8e 100644
> --- a/libavdevice/dshow.c
> +++ b/libavdevice/dshow.c
> @@ -51,7 +51,7 @@ static enum AVPixelFormat dshow_pixfmt(DWORD biCompression, WORD biBitCount)
>                  return AV_PIX_FMT_0RGB32;
>          }
>      }
> -    return avpriv_find_pix_fmt(avpriv_get_raw_pix_fmt_tags(), biCompression); // all others
> +    return avpriv_pix_fmt_find(PIX_FMT_LIST_RAW, biCompression); // all others
>  }
>  
>  static int
> diff --git a/libavformat/avienc.c b/libavformat/avienc.c
> index 3fbde0be1e..be2493ce55 100644
> --- a/libavformat/avienc.c
> +++ b/libavformat/avienc.c
> @@ -448,7 +448,7 @@ static int avi_write_header(AVFormatContext *s)
>                      par->bits_per_coded_sample = 16;
>                  avist->pal_offset = avio_tell(pb) + 40;
>                  ff_put_bmp_header(pb, par, 0, 0, avi->flipped_raw_rgb);
> -                pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
> +                pix_fmt = avpriv_pix_fmt_find(PIX_FMT_LIST_AVI,
>                                                par->bits_per_coded_sample);
>                  if (   !par->codec_tag
>                      && par->codec_id == AV_CODEC_ID_RAWVIDEO
> diff --git a/libavformat/demux.c b/libavformat/demux.c
> index 745dc8687c..360abd6014 100644
> --- a/libavformat/demux.c
> +++ b/libavformat/demux.c
> @@ -2795,7 +2795,7 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options)
>          if (avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
>              if (avctx->codec_id == AV_CODEC_ID_RAWVIDEO && !avctx->codec_tag && !avctx->bits_per_coded_sample) {
>                  uint32_t tag= avcodec_pix_fmt_to_codec_tag(avctx->pix_fmt);
> -                if (avpriv_find_pix_fmt(avpriv_get_raw_pix_fmt_tags(), tag) == avctx->pix_fmt)
> +                if (avpriv_pix_fmt_find(PIX_FMT_LIST_RAW, tag) == avctx->pix_fmt)
>                      avctx->codec_tag= tag;
>              }
>  
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 0f912dd012..7216331fa1 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -1636,7 +1636,7 @@ static int mov_get_rawvideo_codec_tag(AVFormatContext *s, MOVTrack *track)
>          }
>      }
>  
> -    pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_mov,
> +    pix_fmt = avpriv_pix_fmt_find(PIX_FMT_LIST_MOV,
>                                    track->par->bits_per_coded_sample);
>      if (tag == MKTAG('r','a','w',' ') &&
>          track->par->format != pix_fmt &&
> 

Ping for this patchset. (I already applied the trivial first patch.)

- Andreas
_______________________________________________
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".