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

[Stanislav Ionascu]

Hi,

thanks for looking into this.

On Sun, Aug 18, 2019 at 4:55 AM Andreas Rheinhardt
<andreas.rheinhardt at gmail.com> wrote:
>
> 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.

Done.

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

Added the check.

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

Indeed. I've corrected that. Also remuxed all mov's into mkv's, and
verified that they all still work.

>
> > +            }
> > +
> > +            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.

Removed it, as stsd parser will also trace the FourCC code.

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

Yes. Removed as it's not necessary.

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

Removed. The stsd parser takes care of that. It looks for the hvcC atom, and
correctly marks the stream as HEVC if it is found.

I've uploaded a sample via vlc uploader (as
unplayable_dolby_vision_v_quicktime_track),
not sure where it ended up.

Latest patch attached.

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

Thanks!
Stan.


> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
Type: application/octet-stream
Size: 4633 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190818/925df1ee/attachment.obj>