[FFmpeg-devel] [PATCH 1/1] avformat/matroska: fully parse stsd atom in v_quicktime tracks

[Andreas Rheinhardt]

Hello,

I am no expert on mov (and so this should definitely be looked at from
someone who is), but I have some points:

Stanislav Ionascu:
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 1ea9b807e6..2a397c909a 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -2473,25 +2473,58 @@ static int
matroska_parse_tracks(AVFormatContext *s)
>          } else if (!strcmp(track->codec_id, "V_QUICKTIME") &&
>                     (track->codec_priv.size >= 21)          &&
>                     (track->codec_priv.data)) {
> +            MOVStreamContext *msc;
> +            MOVContext *mc = NULL;
> +            void *priv_data;
> +            int nb_streams;

You could initialize nb_streams and priv_data here. And the
initialization of mc is unnecessary.

>              int ret = get_qt_codec(track, &fourcc, &codec_id);
>              if (ret < 0)
>                  return ret;
> -            if (codec_id == AV_CODEC_ID_NONE &&
AV_RL32(track->codec_priv.data+4) == AV_RL32("SMI ")) {
> -                fourcc = MKTAG('S','V','Q','3');
> -                codec_id = ff_codec_get_id(ff_codec_movvideo_tags,
fourcc);
> +            mc = av_mallocz(sizeof(*mc));
> +            if (!mc)
> +                return AVERROR(ENOMEM);
> +            priv_data = st->priv_data;
> +            nb_streams = s->nb_streams;
> +            mc->fc = s;
> +            st->priv_data = msc = av_mallocz(sizeof(MOVStreamContext));
> +            if (!msc) {
> +                av_free(mc);
> +                st->priv_data = priv_data;
> +                return AVERROR(ENOMEM);
>              }
> +            ffio_init_context(&b, track->codec_priv.data,
> +                              track->codec_priv.size,
> +                              0, NULL, NULL, NULL, NULL);
> +
> +            /* ff_mov_read_stsd_entries updates stream s->nb_streams-1,
> +             * so set it temporarily to indicate which stream to
update. */
> +            s->nb_streams = st->index + 1;
> +            ff_mov_read_stsd_entries(mc, &b, 1);

Is it intentional that you don't check the return value here?

> +
> +            /* copy palette from MOVStreamContext */
> +            track->has_palette = msc->has_palette;
> +            if (track->has_palette) {
> +                /* leave bit_depth = -1, to reuse
bits_per_coded_sample  */
> +                memcpy(track->palette, msc->palette, AVPALETTE_COUNT);

In case the track has a palette, your patch would only copy 256 bytes
of it, but a palette consists of 256 uint32_t. (This link
https://drive.google.com/drive/folders/0B3_pEBoLs0faWElmM2FnLTZYNlk
from the commit message that added the palette stuff seems to still
work if you need samples for this.)

> +            }
> +
> +            av_free(msc);
> +            av_free(mc);
> +            st->priv_data = priv_data;
> +            s->nb_streams = nb_streams;
> +            fourcc = st->codecpar->codec_tag;
> +            codec_id = st->codecpar->codec_id;
> +
> +            av_log(matroska->ctx, AV_LOG_TRACE,
> +                   "mov FourCC found %s.\n", av_fourcc2str(fourcc));
> +
>              if (codec_id == AV_CODEC_ID_NONE)
>                  av_log(matroska->ctx, AV_LOG_ERROR,
> -                       "mov FourCC not found %s.\n",
av_fourcc2str(fourcc));

If the codec_id turns out to be AV_CODEC_ID_NONE, two strings will be
output (at least on loglevel trace): "mov FourCC found %s.\n" and then
"mov FourCC not found %s.\n". The first of these is superfluous in
this case.

> -            if (track->codec_priv.size >= 86) {
> -                bit_depth = AV_RB16(track->codec_priv.data + 82);
> -                ffio_init_context(&b, track->codec_priv.data,
> -                                  track->codec_priv.size,
> -                                  0, NULL, NULL, NULL, NULL);
> -                if (ff_get_qtpalette(codec_id, &b, track->palette)) {

If I am not extremely mistaken, then there is no need to include
qtpalette.h any more after removing this function call.

> -                    bit_depth &= 0x1F;
> -                    track->has_palette = 1;
> -                }
> +                        "mov FourCC not found %s.\n",
av_fourcc2str(fourcc));
> +
> +            // dvh1 in mkv is likely HEVC
> +            if (fourcc == MKTAG('d','v','h','1')) {
> +                codec_id = AV_CODEC_ID_HEVC;

Your changes for dvh1 should probably be moved to a separate commit.

>              }
>          } else if (codec_id == AV_CODEC_ID_PCM_S16BE) {
>              switch (track->audio.bitdepth) {

- Andreas