[FFmpeg-devel] [PATCH 2/5] avformat/matroskadec: Parse dvcC/dvvC block additional mapping

[Andreas Rheinhardt]

quietvoid:
> The parsing was implemented in isom, for the unification of
> the mov/mpegts DOVI parsing into one function, in a later patch.
> Most of the implementation was done by Plex developers.
> 
> Signed-off-by: quietvoid <tcChlisop0 at gmail.com>
> ---
>  libavformat/isom.c        | 51 +++++++++++++++++++++++++++++++++++++++
>  libavformat/isom.h        |  5 ++++
>  libavformat/matroskadec.c | 13 ++++++++++
>  3 files changed, 69 insertions(+)
> 
> diff --git a/libavformat/isom.c b/libavformat/isom.c
> index 4df5440023..80a8f4d7b0 100644
> --- a/libavformat/isom.c
> +++ b/libavformat/isom.c
> @@ -430,3 +430,54 @@ void ff_mov_write_chan(AVIOContext *pb, int64_t channel_layout)
>      }
>      avio_wb32(pb, 0);              // mNumberChannelDescriptions
>  }
> +
> +int ff_mov_parse_dvcc_dvvc(AVFormatContext *s, AVStream *st, GetBitContext *gb)
> +{
> +    AVDOVIDecoderConfigurationRecord *dovi;
> +    size_t dovi_size;
> +    int ret;
> +
> +    if (gb->size_in_bits < 32)
> +        return AVERROR_INVALIDDATA;
> +
> +    dovi = av_dovi_alloc(&dovi_size);
> +    if (!dovi)
> +        return AVERROR(ENOMEM);
> +
> +    dovi->dv_version_major = get_bits(gb, 8);
> +    dovi->dv_version_minor = get_bits(gb, 8);
> +
> +    dovi->dv_profile        = get_bits(gb, 7);
> +    dovi->dv_level          = get_bits(gb, 6);
> +    dovi->rpu_present_flag  = get_bits1(gb);
> +    dovi->el_present_flag   = get_bits1(gb);
> +    dovi->bl_present_flag   = get_bits1(gb);
> +
> +    // Has enough remaining data
> +    if (gb->size_in_bits >= 36) {
> +        dovi->dv_bl_signal_compatibility_id = get_bits(gb, 4);
> +    } else {
> +        // 0 stands for None
> +        // Dolby Vision V1.2.93 profiles and levels
> +        dovi->dv_bl_signal_compatibility_id = 0;
> +    }
> +
> +    ret = av_stream_add_side_data(st, AV_PKT_DATA_DOVI_CONF,
> +                                  (uint8_t *)dovi, dovi_size);
> +    if (ret < 0) {
> +        av_free(dovi);
> +        return ret;
> +    }
> +
> +    av_log(s, AV_LOG_TRACE, "DOVI, version: %d.%d, profile: %d, level: %d, "
> +           "rpu flag: %d, el flag: %d, bl flag: %d, compatibility id: %d\n",
> +           dovi->dv_version_major, dovi->dv_version_minor,
> +           dovi->dv_profile, dovi->dv_level,
> +           dovi->rpu_present_flag,
> +           dovi->el_present_flag,
> +           dovi->bl_present_flag,
> +           dovi->dv_bl_signal_compatibility_id
> +        );
> +
> +    return 0;
> +}

The Matroska demuxer currently needs nothing from isom.c and so has no
Makefile dependency for it. So a build with only the Matroska demuxer
enabled will fail. I'd like to avoid a dependency on the unused parts of
isom.c (isom.c calls some avpriv functions from libavcodec and I'd
really like to avoid this unnecessary dependency), so can you move this
into a new file (say isom_dovi.c or so)?

> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index 34a58c79b7..44bd839852 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -27,6 +27,9 @@
>  #include <stddef.h>
>  #include <stdint.h>
>  
> +#include "libavcodec/get_bits.h"
> +
> +#include "libavutil/dovi_meta.h"
>  #include "libavutil/encryption_info.h"
>  #include "libavutil/mastering_display_metadata.h"
>  #include "libavutil/spherical.h"
> @@ -390,4 +393,6 @@ static inline enum AVCodecID ff_mov_get_lpcm_codec_id(int bps, int flags)
>  #define MOV_ISMV_TTML_TAG MKTAG('d', 'f', 'x', 'p')
>  #define MOV_MP4_TTML_TAG  MKTAG('s', 't', 'p', 'p')
>  
> +int ff_mov_parse_dvcc_dvvc(AVFormatContext *s, AVStream *st, GetBitContext *gb);
> +
>  #endif /* AVFORMAT_ISOM_H */
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 8ae553953d..4633067d48 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -2323,6 +2323,14 @@ static int mkv_parse_video_projection(AVStream *st, const MatroskaTrack *track,
>      return 0;
>  }
>  
> +static int mkv_parse_dvcc_dvvc(AVFormatContext *s, AVStream *st, const MatroskaTrack *track,
> +                               EbmlBin *bin)
> +{
> +    GetBitContext gb;
> +    init_get_bits8(&gb, bin->data, bin->size);

I do not see why the caller has to open the GetBitContext instead of
passing a simple buffer. All three prospective users of
ff_mov_parse_dvcc_dvvc have a buffer, but not a GetBitContext to read it.
Furthermore, using a GetBitContext for such a trivial parsing might
backfire: Not all buffers here are padded (the wtvdec demuxer also uses
it via ff_parse_mpeg2_descriptor), so it seems best to just parse it
without a GetBitContext in the way mov already does.

> +    return ff_mov_parse_dvcc_dvvc(s, st, &gb);
> +}
> +
>  static int mkv_parse_block_addition_mappings(AVFormatContext *s, AVStream *st, const MatroskaTrack *track)
>  {
>      int i, ret;
> @@ -2333,6 +2341,11 @@ static int mkv_parse_block_addition_mappings(AVFormatContext *s, AVStream *st, c
>          MatroskaBlockAdditionMapping *mapping = &mappings[i];
>  
>          switch (mapping->type) {
> +        case MKBETAG('d','v','c','C'):
> +        case MKBETAG('d','v','v','C'):
> +            if ((ret = mkv_parse_dvcc_dvvc(s, st, track, &mapping->extradata)) < 0)
> +                return ret;
> +            break;
>          default:
>              av_log(s, AV_LOG_DEBUG,
>                     "Unknown block additional mapping type %i, value %i, name \"%s\"\n",
>