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

Submitted by Stanislav Ionascu on Aug. 13, 2019, 7:16 p.m.

Details

Message ID 20190813191620.14480-2-stanislav.ionascu@gmail.com
State New
Headers show

Commit Message

Stanislav Ionascu Aug. 13, 2019, 7:16 p.m.
Per matroska spec, v_quicktime contains the complete stsd atom, after
the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
the track, it becomes possible to demux/decode mp4/mov tracks stored as is
in matroska containers.

Also dvh1 in stsd in matroska is more likely hevc codec than dv.

Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
---
 libavformat/matroskadec.c | 51 +++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 15 deletions(-)

Comments

Andreas Rheinhardt Aug. 13, 2019, 8:13 p.m.
Stanislav Ionascu:
> Per matroska spec, v_quicktime contains the complete stsd atom, after
> the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> in matroska containers.
> 
> Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> 
> Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> ---
>  libavformat/matroskadec.c | 51 +++++++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 4e20f15792..88bc89c545 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -2473,25 +2473,46 @@ 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;
> +            AVIOContext *stsd_ctx = NULL;
> +            void *priv_data;
> +            int nb_streams;
>              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);
> +            av_log(matroska->ctx, AV_LOG_TRACE,
> +                   "FourCC found %s.\n", av_fourcc2str(fourcc));
> +            priv_data = st->priv_data;
> +            nb_streams = s->nb_streams;
> +            mc = av_mallocz(sizeof(*mc));
> +            if (!mc)
> +                return AVERROR(ENOMEM);
> +            stsd_ctx = avio_alloc_context(track->codec_priv.data,
> +                                track->codec_priv.size,
> +                                0, NULL, NULL, NULL, NULL);
> +            if (!stsd_ctx)
> +                return AVERROR(ENOMEM);
I haven't looked at this patch deeply yet, but it seems to me that you
should rather use ffio_init_context like it is done in the code that
you intend to delete. That saves allocating and freeing stsd_ctx. You
can even reuse the AVIOContext b that already exists on the stack.
> +            mc->fc = s;
> +            st->priv_data = msc = av_mallocz(sizeof(MOVStreamContext));
> +            if (!msc) {
> +                av_free(mc);
> +                st->priv_data = priv_data;
> +                return AVERROR(ENOMEM);
>              }
> -            if (codec_id == AV_CODEC_ID_NONE)
> -                av_log(matroska->ctx, AV_LOG_ERROR,
> -                       "mov FourCC not found %s.\n", av_fourcc2str(fourcc));
> -            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)) {
> -                    bit_depth &= 0x1F;
> -                    track->has_palette = 1;
Why are you removing this code? What about tracks that ought to have a
palette?
> -                }
> +            /* 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, stsd_ctx, 1);
> +            av_free(msc);
> +            av_free(mc);
> +            avio_context_free(&stsd_ctx);
> +            st->priv_data = priv_data;
> +            s->nb_streams = nb_streams;
> +
> +            // dvh1 in mkv is likely HEVC
> +            if (st->codecpar->codec_tag == MKTAG('d','v','h','1')) {
> +                codec_id = AV_CODEC_ID_HEVC;
>              }
>          } else if (codec_id == AV_CODEC_ID_PCM_S16BE) {
>              switch (track->audio.bitdepth) {
> 

Also, your patch should refer to the exact component that is about to
be changed: avformat/matroskadec.

- Andreas
Stanislav Ionascu Aug. 14, 2019, 6:44 p.m.
On Tue, Aug 13, 2019 at 10:22 PM Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
>
> Stanislav Ionascu:
> > Per matroska spec, v_quicktime contains the complete stsd atom, after
> > the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> > the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> > in matroska containers.
> >
> > Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> >
> > Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > ---
> >  libavformat/matroskadec.c | 51 +++++++++++++++++++++++++++------------
> >  1 file changed, 36 insertions(+), 15 deletions(-)
> >
> > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > index 4e20f15792..88bc89c545 100644
> > --- a/libavformat/matroskadec.c
> > +++ b/libavformat/matroskadec.c
> > @@ -2473,25 +2473,46 @@ 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;
> > +            AVIOContext *stsd_ctx = NULL;
> > +            void *priv_data;
> > +            int nb_streams;
> >              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);
> > +            av_log(matroska->ctx, AV_LOG_TRACE,
> > +                   "FourCC found %s.\n", av_fourcc2str(fourcc));
> > +            priv_data = st->priv_data;
> > +            nb_streams = s->nb_streams;
> > +            mc = av_mallocz(sizeof(*mc));
> > +            if (!mc)
> > +                return AVERROR(ENOMEM);
> > +            stsd_ctx = avio_alloc_context(track->codec_priv.data,
> > +                                track->codec_priv.size,
> > +                                0, NULL, NULL, NULL, NULL);
> > +            if (!stsd_ctx)
> > +                return AVERROR(ENOMEM);
> I haven't looked at this patch deeply yet, but it seems to me that you
> should rather use ffio_init_context like it is done in the code that
> you intend to delete. That saves allocating and freeing stsd_ctx. You
> can even reuse the AVIOContext b that already exists on the stack.

Done.

> > +            mc->fc = s;
> > +            st->priv_data = msc = av_mallocz(sizeof(MOVStreamContext));
> > +            if (!msc) {
> > +                av_free(mc);
> > +                st->priv_data = priv_data;
> > +                return AVERROR(ENOMEM);
> >              }
> > -            if (codec_id == AV_CODEC_ID_NONE)
> > -                av_log(matroska->ctx, AV_LOG_ERROR,
> > -                       "mov FourCC not found %s.\n", av_fourcc2str(fourcc));
> > -            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)) {
> > -                    bit_depth &= 0x1F;
> > -                    track->has_palette = 1;
> Why are you removing this code? What about tracks that ought to have a
> palette?

The palette parsing is done in the stsd parser, but it still has to be
copied back into the track,
which is done in the later patch.

> > -                }
> > +            /* 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, stsd_ctx, 1);
> > +            av_free(msc);
> > +            av_free(mc);
> > +            avio_context_free(&stsd_ctx);
> > +            st->priv_data = priv_data;
> > +            s->nb_streams = nb_streams;
> > +
> > +            // dvh1 in mkv is likely HEVC
> > +            if (st->codecpar->codec_tag == MKTAG('d','v','h','1')) {
> > +                codec_id = AV_CODEC_ID_HEVC;
> >              }
> >          } else if (codec_id == AV_CODEC_ID_PCM_S16BE) {
> >              switch (track->audio.bitdepth) {
> >
>
> Also, your patch should refer to the exact component that is about to
> be changed: avformat/matroskadec.

Done.

>
> - 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".
Michael Niedermayer Aug. 14, 2019, 9:50 p.m.
On Wed, Aug 14, 2019 at 08:44:11PM +0200, Stanislav Ionascu wrote:
> On Tue, Aug 13, 2019 at 10:22 PM Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com> wrote:
> >
> > Stanislav Ionascu:
> > > Per matroska spec, v_quicktime contains the complete stsd atom, after
> > > the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> > > the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> > > in matroska containers.
> > >
> > > Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> > >
> > > Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > > ---
> > >  libavformat/matroskadec.c | 51 +++++++++++++++++++++++++++------------
> > >  1 file changed, 36 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > > index 4e20f15792..88bc89c545 100644
> > > --- a/libavformat/matroskadec.c
> > > +++ b/libavformat/matroskadec.c
> > > @@ -2473,25 +2473,46 @@ 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;
> > > +            AVIOContext *stsd_ctx = NULL;
> > > +            void *priv_data;
> > > +            int nb_streams;
> > >              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);
> > > +            av_log(matroska->ctx, AV_LOG_TRACE,
> > > +                   "FourCC found %s.\n", av_fourcc2str(fourcc));
> > > +            priv_data = st->priv_data;
> > > +            nb_streams = s->nb_streams;
> > > +            mc = av_mallocz(sizeof(*mc));
> > > +            if (!mc)
> > > +                return AVERROR(ENOMEM);
> > > +            stsd_ctx = avio_alloc_context(track->codec_priv.data,
> > > +                                track->codec_priv.size,
> > > +                                0, NULL, NULL, NULL, NULL);
> > > +            if (!stsd_ctx)
> > > +                return AVERROR(ENOMEM);
> > I haven't looked at this patch deeply yet, but it seems to me that you
> > should rather use ffio_init_context like it is done in the code that
> > you intend to delete. That saves allocating and freeing stsd_ctx. You
> > can even reuse the AVIOContext b that already exists on the stack.
> 
> Done.
> 
> > > +            mc->fc = s;
> > > +            st->priv_data = msc = av_mallocz(sizeof(MOVStreamContext));
> > > +            if (!msc) {
> > > +                av_free(mc);
> > > +                st->priv_data = priv_data;
> > > +                return AVERROR(ENOMEM);
> > >              }
> > > -            if (codec_id == AV_CODEC_ID_NONE)
> > > -                av_log(matroska->ctx, AV_LOG_ERROR,
> > > -                       "mov FourCC not found %s.\n", av_fourcc2str(fourcc));
> > > -            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)) {
> > > -                    bit_depth &= 0x1F;
> > > -                    track->has_palette = 1;
> > Why are you removing this code? What about tracks that ought to have a
> > palette?
> 
> The palette parsing is done in the stsd parser, but it still has to be
> copied back into the track,
> which is done in the later patch.
> 
> > > -                }
> > > +            /* 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, stsd_ctx, 1);
> > > +            av_free(msc);
> > > +            av_free(mc);
> > > +            avio_context_free(&stsd_ctx);
> > > +            st->priv_data = priv_data;
> > > +            s->nb_streams = nb_streams;
> > > +
> > > +            // dvh1 in mkv is likely HEVC
> > > +            if (st->codecpar->codec_tag == MKTAG('d','v','h','1')) {
> > > +                codec_id = AV_CODEC_ID_HEVC;
> > >              }
> > >          } else if (codec_id == AV_CODEC_ID_PCM_S16BE) {
> > >              switch (track->audio.bitdepth) {
> > >
> >
> > Also, your patch should refer to the exact component that is about to
> > be changed: avformat/matroskadec.
> 
> Done.
> 
> >
> > - 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".

>  matroskadec.c |   61 ++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 43 insertions(+), 18 deletions(-)
> 5dbf0d633a4567ad24eb07356d5de3c4a67b19e0  0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
> From 18d1e18e3c72f70c2d9aaf2e8d9270a28b4895a2 Mon Sep 17 00:00:00 2001
> From: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> Date: Sun, 11 Aug 2019 21:10:30 +0200
> Subject: [PATCH] avformat/matroskadec: properly parse stsd in v_quicktime
> 
> Per matroska spec, v_quicktime contains the complete stsd atom, after
> the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> in matroska containers.
> 
> Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> QuickTime palette parsing is reused from the stsd parser results.
> 
> Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> ---
>  libavformat/matroskadec.c | 61 +++++++++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 18 deletions(-)

breaks ./ffmpeg -i Earth\ Spin\ 1-bit\ qtrle.mkv -f null -
link to the file is in bf42a7ef6d073221915dcc042c080374045ab245 (and still works as of this writing)

thx

[...]
Stanislav Ionascu Aug. 15, 2019, 5:59 a.m.
Hi,

On Wed, Aug 14, 2019 at 11:50 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Wed, Aug 14, 2019 at 08:44:11PM +0200, Stanislav Ionascu wrote:
> > On Tue, Aug 13, 2019 at 10:22 PM Andreas Rheinhardt
> > <andreas.rheinhardt@gmail.com> wrote:
> > >
> > > Stanislav Ionascu:
> > > > Per matroska spec, v_quicktime contains the complete stsd atom, after
> > > > the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> > > > the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> > > > in matroska containers.
> > > >
> > > > Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> > > >
> > > > Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > > > ---
> > > >  libavformat/matroskadec.c | 51 +++++++++++++++++++++++++++------------
> > > >  1 file changed, 36 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > > > index 4e20f15792..88bc89c545 100644
> > > > --- a/libavformat/matroskadec.c
> > > > +++ b/libavformat/matroskadec.c
> > > > @@ -2473,25 +2473,46 @@ 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;
> > > > +            AVIOContext *stsd_ctx = NULL;
> > > > +            void *priv_data;
> > > > +            int nb_streams;
> > > >              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);
> > > > +            av_log(matroska->ctx, AV_LOG_TRACE,
> > > > +                   "FourCC found %s.\n", av_fourcc2str(fourcc));
> > > > +            priv_data = st->priv_data;
> > > > +            nb_streams = s->nb_streams;
> > > > +            mc = av_mallocz(sizeof(*mc));
> > > > +            if (!mc)
> > > > +                return AVERROR(ENOMEM);
> > > > +            stsd_ctx = avio_alloc_context(track->codec_priv.data,
> > > > +                                track->codec_priv.size,
> > > > +                                0, NULL, NULL, NULL, NULL);
> > > > +            if (!stsd_ctx)
> > > > +                return AVERROR(ENOMEM);
> > > I haven't looked at this patch deeply yet, but it seems to me that you
> > > should rather use ffio_init_context like it is done in the code that
> > > you intend to delete. That saves allocating and freeing stsd_ctx. You
> > > can even reuse the AVIOContext b that already exists on the stack.
> >
> > Done.
> >
> > > > +            mc->fc = s;
> > > > +            st->priv_data = msc = av_mallocz(sizeof(MOVStreamContext));
> > > > +            if (!msc) {
> > > > +                av_free(mc);
> > > > +                st->priv_data = priv_data;
> > > > +                return AVERROR(ENOMEM);
> > > >              }
> > > > -            if (codec_id == AV_CODEC_ID_NONE)
> > > > -                av_log(matroska->ctx, AV_LOG_ERROR,
> > > > -                       "mov FourCC not found %s.\n", av_fourcc2str(fourcc));
> > > > -            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)) {
> > > > -                    bit_depth &= 0x1F;
> > > > -                    track->has_palette = 1;
> > > Why are you removing this code? What about tracks that ought to have a
> > > palette?
> >
> > The palette parsing is done in the stsd parser, but it still has to be
> > copied back into the track,
> > which is done in the later patch.
> >
> > > > -                }
> > > > +            /* 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, stsd_ctx, 1);
> > > > +            av_free(msc);
> > > > +            av_free(mc);
> > > > +            avio_context_free(&stsd_ctx);
> > > > +            st->priv_data = priv_data;
> > > > +            s->nb_streams = nb_streams;
> > > > +
> > > > +            // dvh1 in mkv is likely HEVC
> > > > +            if (st->codecpar->codec_tag == MKTAG('d','v','h','1')) {
> > > > +                codec_id = AV_CODEC_ID_HEVC;
> > > >              }
> > > >          } else if (codec_id == AV_CODEC_ID_PCM_S16BE) {
> > > >              switch (track->audio.bitdepth) {
> > > >
> > >
> > > Also, your patch should refer to the exact component that is about to
> > > be changed: avformat/matroskadec.
> >
> > Done.
> >
> > >
> > > - 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".
>
> >  matroskadec.c |   61 ++++++++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 43 insertions(+), 18 deletions(-)
> > 5dbf0d633a4567ad24eb07356d5de3c4a67b19e0  0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
> > From 18d1e18e3c72f70c2d9aaf2e8d9270a28b4895a2 Mon Sep 17 00:00:00 2001
> > From: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > Date: Sun, 11 Aug 2019 21:10:30 +0200
> > Subject: [PATCH] avformat/matroskadec: properly parse stsd in v_quicktime
> >
> > Per matroska spec, v_quicktime contains the complete stsd atom, after
> > the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> > the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> > in matroska containers.
> >
> > Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> > QuickTime palette parsing is reused from the stsd parser results.
> >
> > Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > ---
> >  libavformat/matroskadec.c | 61 +++++++++++++++++++++++++++------------
> >  1 file changed, 43 insertions(+), 18 deletions(-)
>
> breaks ./ffmpeg -i Earth\ Spin\ 1-bit\ qtrle.mkv -f null -
> link to the file is in bf42a7ef6d073221915dcc042c080374045ab245 (and still works as of this writing)
>

My bad. Should've stored temporarily the codec_id from codecpar, as it
will write it back.

Thanks!



> thx
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The educated differ from the uneducated as much as the living from the
> dead. -- Aristotle
> _______________________________________________
> 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".
Michael Niedermayer Aug. 16, 2019, 10:53 p.m.
On Thu, Aug 15, 2019 at 07:59:28AM +0200, Stanislav Ionascu wrote:
> Hi,
> 
> On Wed, Aug 14, 2019 at 11:50 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > On Wed, Aug 14, 2019 at 08:44:11PM +0200, Stanislav Ionascu wrote:
> > > On Tue, Aug 13, 2019 at 10:22 PM Andreas Rheinhardt
> > > <andreas.rheinhardt@gmail.com> wrote:
> > > >
> > > > Stanislav Ionascu:
> > > > > Per matroska spec, v_quicktime contains the complete stsd atom, after
> > > > > the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> > > > > the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> > > > > in matroska containers.
> > > > >
> > > > > Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> > > > >
> > > > > Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > > > > ---
> > > > >  libavformat/matroskadec.c | 51 +++++++++++++++++++++++++++------------
> > > > >  1 file changed, 36 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > > > > index 4e20f15792..88bc89c545 100644
> > > > > --- a/libavformat/matroskadec.c
> > > > > +++ b/libavformat/matroskadec.c
> > > > > @@ -2473,25 +2473,46 @@ 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;
> > > > > +            AVIOContext *stsd_ctx = NULL;
> > > > > +            void *priv_data;
> > > > > +            int nb_streams;
> > > > >              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);
> > > > > +            av_log(matroska->ctx, AV_LOG_TRACE,
> > > > > +                   "FourCC found %s.\n", av_fourcc2str(fourcc));
> > > > > +            priv_data = st->priv_data;
> > > > > +            nb_streams = s->nb_streams;
> > > > > +            mc = av_mallocz(sizeof(*mc));
> > > > > +            if (!mc)
> > > > > +                return AVERROR(ENOMEM);
> > > > > +            stsd_ctx = avio_alloc_context(track->codec_priv.data,
> > > > > +                                track->codec_priv.size,
> > > > > +                                0, NULL, NULL, NULL, NULL);
> > > > > +            if (!stsd_ctx)
> > > > > +                return AVERROR(ENOMEM);
> > > > I haven't looked at this patch deeply yet, but it seems to me that you
> > > > should rather use ffio_init_context like it is done in the code that
> > > > you intend to delete. That saves allocating and freeing stsd_ctx. You
> > > > can even reuse the AVIOContext b that already exists on the stack.
> > >
> > > Done.
> > >
> > > > > +            mc->fc = s;
> > > > > +            st->priv_data = msc = av_mallocz(sizeof(MOVStreamContext));
> > > > > +            if (!msc) {
> > > > > +                av_free(mc);
> > > > > +                st->priv_data = priv_data;
> > > > > +                return AVERROR(ENOMEM);
> > > > >              }
> > > > > -            if (codec_id == AV_CODEC_ID_NONE)
> > > > > -                av_log(matroska->ctx, AV_LOG_ERROR,
> > > > > -                       "mov FourCC not found %s.\n", av_fourcc2str(fourcc));
> > > > > -            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)) {
> > > > > -                    bit_depth &= 0x1F;
> > > > > -                    track->has_palette = 1;
> > > > Why are you removing this code? What about tracks that ought to have a
> > > > palette?
> > >
> > > The palette parsing is done in the stsd parser, but it still has to be
> > > copied back into the track,
> > > which is done in the later patch.
> > >
> > > > > -                }
> > > > > +            /* 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, stsd_ctx, 1);
> > > > > +            av_free(msc);
> > > > > +            av_free(mc);
> > > > > +            avio_context_free(&stsd_ctx);
> > > > > +            st->priv_data = priv_data;
> > > > > +            s->nb_streams = nb_streams;
> > > > > +
> > > > > +            // dvh1 in mkv is likely HEVC
> > > > > +            if (st->codecpar->codec_tag == MKTAG('d','v','h','1')) {
> > > > > +                codec_id = AV_CODEC_ID_HEVC;
> > > > >              }
> > > > >          } else if (codec_id == AV_CODEC_ID_PCM_S16BE) {
> > > > >              switch (track->audio.bitdepth) {
> > > > >
> > > >
> > > > Also, your patch should refer to the exact component that is about to
> > > > be changed: avformat/matroskadec.
> > >
> > > Done.
> > >
> > > >
> > > > - 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".
> >
> > >  matroskadec.c |   61 ++++++++++++++++++++++++++++++++++++++++------------------
> > >  1 file changed, 43 insertions(+), 18 deletions(-)
> > > 5dbf0d633a4567ad24eb07356d5de3c4a67b19e0  0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
> > > From 18d1e18e3c72f70c2d9aaf2e8d9270a28b4895a2 Mon Sep 17 00:00:00 2001
> > > From: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > > Date: Sun, 11 Aug 2019 21:10:30 +0200
> > > Subject: [PATCH] avformat/matroskadec: properly parse stsd in v_quicktime
> > >
> > > Per matroska spec, v_quicktime contains the complete stsd atom, after
> > > the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> > > the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> > > in matroska containers.
> > >
> > > Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> > > QuickTime palette parsing is reused from the stsd parser results.
> > >
> > > Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > > ---
> > >  libavformat/matroskadec.c | 61 +++++++++++++++++++++++++++------------
> > >  1 file changed, 43 insertions(+), 18 deletions(-)
> >
> > breaks ./ffmpeg -i Earth\ Spin\ 1-bit\ qtrle.mkv -f null -
> > link to the file is in bf42a7ef6d073221915dcc042c080374045ab245 (and still works as of this writing)
> >
> 
> My bad. Should've stored temporarily the codec_id from codecpar, as it
> will write it back.
> 
> Thanks!
> 
> 
> 
> > thx
> >
> > [...]
> >
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > The educated differ from the uneducated as much as the living from the
> > dead. -- Aristotle
> > _______________________________________________
> > 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".

>  matroskadec.c |   62 +++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 44 insertions(+), 18 deletions(-)
> a1ae98fa5d8dabe59ff6c3c8822aaef19ce980e6  0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
> From 9d639bea980bb7940bfeb527a60c847c14d13ee9 Mon Sep 17 00:00:00 2001
> From: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> Date: Sun, 11 Aug 2019 21:10:30 +0200
> Subject: [PATCH] avformat/matroskadec: properly parse stsd in v_quicktime
> 
> Per matroska spec, v_quicktime contains the complete stsd atom, after
> the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> in matroska containers.
> 
> Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> QuickTime palette parsing is reused from the stsd parser results.
> 
> Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> ---
>  libavformat/matroskadec.c | 62 +++++++++++++++++++++++++++------------
>  1 file changed, 44 insertions(+), 18 deletions(-)

breaks:
./ffmpeg -i tickets//3256/output-ffmpeg-20120804-git-f857465.mkv -f null -

https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket3256/output-ffmpeg-20120804-git-f857465.mkv

[...]
Stanislav Ionascu Aug. 17, 2019, 10:38 a.m.
Hi!

On Sat, Aug 17, 2019 at 12:53 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Thu, Aug 15, 2019 at 07:59:28AM +0200, Stanislav Ionascu wrote:
> > Hi,
> >
> > On Wed, Aug 14, 2019 at 11:50 PM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > On Wed, Aug 14, 2019 at 08:44:11PM +0200, Stanislav Ionascu wrote:
> > > > On Tue, Aug 13, 2019 at 10:22 PM Andreas Rheinhardt
> > > > <andreas.rheinhardt@gmail.com> wrote:
> > > > >
> > > > > Stanislav Ionascu:
> > > > > > Per matroska spec, v_quicktime contains the complete stsd atom, after
> > > > > > the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> > > > > > the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> > > > > > in matroska containers.
> > > > > >
> > > > > > Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> > > > > >
> > > > > > Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > > > > > ---
> > > > > >  libavformat/matroskadec.c | 51 +++++++++++++++++++++++++++------------
> > > > > >  1 file changed, 36 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > > > > > index 4e20f15792..88bc89c545 100644
> > > > > > --- a/libavformat/matroskadec.c
> > > > > > +++ b/libavformat/matroskadec.c
> > > > > > @@ -2473,25 +2473,46 @@ 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;
> > > > > > +            AVIOContext *stsd_ctx = NULL;
> > > > > > +            void *priv_data;
> > > > > > +            int nb_streams;
> > > > > >              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);
> > > > > > +            av_log(matroska->ctx, AV_LOG_TRACE,
> > > > > > +                   "FourCC found %s.\n", av_fourcc2str(fourcc));
> > > > > > +            priv_data = st->priv_data;
> > > > > > +            nb_streams = s->nb_streams;
> > > > > > +            mc = av_mallocz(sizeof(*mc));
> > > > > > +            if (!mc)
> > > > > > +                return AVERROR(ENOMEM);
> > > > > > +            stsd_ctx = avio_alloc_context(track->codec_priv.data,
> > > > > > +                                track->codec_priv.size,
> > > > > > +                                0, NULL, NULL, NULL, NULL);
> > > > > > +            if (!stsd_ctx)
> > > > > > +                return AVERROR(ENOMEM);
> > > > > I haven't looked at this patch deeply yet, but it seems to me that you
> > > > > should rather use ffio_init_context like it is done in the code that
> > > > > you intend to delete. That saves allocating and freeing stsd_ctx. You
> > > > > can even reuse the AVIOContext b that already exists on the stack.
> > > >
> > > > Done.
> > > >
> > > > > > +            mc->fc = s;
> > > > > > +            st->priv_data = msc = av_mallocz(sizeof(MOVStreamContext));
> > > > > > +            if (!msc) {
> > > > > > +                av_free(mc);
> > > > > > +                st->priv_data = priv_data;
> > > > > > +                return AVERROR(ENOMEM);
> > > > > >              }
> > > > > > -            if (codec_id == AV_CODEC_ID_NONE)
> > > > > > -                av_log(matroska->ctx, AV_LOG_ERROR,
> > > > > > -                       "mov FourCC not found %s.\n", av_fourcc2str(fourcc));
> > > > > > -            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)) {
> > > > > > -                    bit_depth &= 0x1F;
> > > > > > -                    track->has_palette = 1;
> > > > > Why are you removing this code? What about tracks that ought to have a
> > > > > palette?
> > > >
> > > > The palette parsing is done in the stsd parser, but it still has to be
> > > > copied back into the track,
> > > > which is done in the later patch.
> > > >
> > > > > > -                }
> > > > > > +            /* 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, stsd_ctx, 1);
> > > > > > +            av_free(msc);
> > > > > > +            av_free(mc);
> > > > > > +            avio_context_free(&stsd_ctx);
> > > > > > +            st->priv_data = priv_data;
> > > > > > +            s->nb_streams = nb_streams;
> > > > > > +
> > > > > > +            // dvh1 in mkv is likely HEVC
> > > > > > +            if (st->codecpar->codec_tag == MKTAG('d','v','h','1')) {
> > > > > > +                codec_id = AV_CODEC_ID_HEVC;
> > > > > >              }
> > > > > >          } else if (codec_id == AV_CODEC_ID_PCM_S16BE) {
> > > > > >              switch (track->audio.bitdepth) {
> > > > > >
> > > > >
> > > > > Also, your patch should refer to the exact component that is about to
> > > > > be changed: avformat/matroskadec.
> > > >
> > > > Done.
> > > >
> > > > >
> > > > > - 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".
> > >
> > > >  matroskadec.c |   61 ++++++++++++++++++++++++++++++++++++++++------------------
> > > >  1 file changed, 43 insertions(+), 18 deletions(-)
> > > > 5dbf0d633a4567ad24eb07356d5de3c4a67b19e0  0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
> > > > From 18d1e18e3c72f70c2d9aaf2e8d9270a28b4895a2 Mon Sep 17 00:00:00 2001
> > > > From: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > > > Date: Sun, 11 Aug 2019 21:10:30 +0200
> > > > Subject: [PATCH] avformat/matroskadec: properly parse stsd in v_quicktime
> > > >
> > > > Per matroska spec, v_quicktime contains the complete stsd atom, after
> > > > the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> > > > the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> > > > in matroska containers.
> > > >
> > > > Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> > > > QuickTime palette parsing is reused from the stsd parser results.
> > > >
> > > > Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > > > ---
> > > >  libavformat/matroskadec.c | 61 +++++++++++++++++++++++++++------------
> > > >  1 file changed, 43 insertions(+), 18 deletions(-)
> > >
> > > breaks ./ffmpeg -i Earth\ Spin\ 1-bit\ qtrle.mkv -f null -
> > > link to the file is in bf42a7ef6d073221915dcc042c080374045ab245 (and still works as of this writing)
> > >
> >
> > My bad. Should've stored temporarily the codec_id from codecpar, as it
> > will write it back.
> >
> > Thanks!
> >
> >
> >
> > > thx
> > >
> > > [...]
> > >
> > > --
> > > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> > >
> > > The educated differ from the uneducated as much as the living from the
> > > dead. -- Aristotle
> > > _______________________________________________
> > > 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".
>
> >  matroskadec.c |   62 +++++++++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 44 insertions(+), 18 deletions(-)
> > a1ae98fa5d8dabe59ff6c3c8822aaef19ce980e6  0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
> > From 9d639bea980bb7940bfeb527a60c847c14d13ee9 Mon Sep 17 00:00:00 2001
> > From: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > Date: Sun, 11 Aug 2019 21:10:30 +0200
> > Subject: [PATCH] avformat/matroskadec: properly parse stsd in v_quicktime
> >
> > Per matroska spec, v_quicktime contains the complete stsd atom, after
> > the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> > the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> > in matroska containers.
> >
> > Also dvh1 in stsd in matroska is more likely hevc codec than dv.
> > QuickTime palette parsing is reused from the stsd parser results.
> >
> > Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > ---
> >  libavformat/matroskadec.c | 62 +++++++++++++++++++++++++++------------
> >  1 file changed, 44 insertions(+), 18 deletions(-)
>
> breaks:
> ./ffmpeg -i tickets//3256/output-ffmpeg-20120804-git-f857465.mkv -f null -
>
> https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket3256/output-ffmpeg-20120804-git-f857465.mkv

Thanks. Brought back the workaround for the non-compliant private track data.
Patch attached.

Stan.



>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Good people do not need laws to tell them to act responsibly, while bad
> people will find a way around the laws. -- Plato
> _______________________________________________
> 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".
Andreas Rheinhardt Aug. 18, 2019, 2:47 a.m.
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
Stanislav Ionascu Aug. 18, 2019, 10:32 a.m.
Hi,

thanks for looking into this.

On Sun, Aug 18, 2019 at 4:55 AM Andreas Rheinhardt
<andreas.rheinhardt@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Aug. 20, 2019, 8:19 a.m.
On Sun, Aug 18, 2019 at 12:32:03PM +0200, Stanislav Ionascu wrote:
> Hi,
> 
> thanks for looking into this.
> 
> On Sun, Aug 18, 2019 at 4:55 AM Andreas Rheinhardt
> <andreas.rheinhardt@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@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

>  matroskadec.c |   62 +++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 46 insertions(+), 16 deletions(-)
> 6ce3d41fa68f1cc46aec967182bc39b1a72f353d  0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
> From b817065fb345c0075347235daed806ab8488e502 Mon Sep 17 00:00:00 2001
> From: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> Date: Sun, 11 Aug 2019 21:10:30 +0200
> Subject: [PATCH] avformat/matroskadec: properly parse stsd in v_quicktime
> 
> Per matroska spec, v_quicktime contains the complete stsd atom, after
> the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> in matroska containers.
> 
> QuickTime palette parsing is reused from the stsd parser results.
> For non compliant input (when fourcc comes without the size), shift and
> prepend the size to the private data.
> Reading the SMI atom is reused from the stsd parser.
> 
> Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>

seems to break:
./ffmpeg -i tickets/3256/output-ffmpeg-20140109-git-c0a33c4.mkv -t 2 f.mp4

https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket3256/output-ffmpeg-20140109-git-c0a33c4.mkv

[...]
Stanislav Ionascu Aug. 22, 2019, 5:59 a.m.
Hi!

On Tue, Aug 20, 2019 at 10:19 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sun, Aug 18, 2019 at 12:32:03PM +0200, Stanislav Ionascu wrote:
> > Hi,
> >
> > thanks for looking into this.
> >
> > On Sun, Aug 18, 2019 at 4:55 AM Andreas Rheinhardt
> > <andreas.rheinhardt@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@ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
> >  matroskadec.c |   62 +++++++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 46 insertions(+), 16 deletions(-)
> > 6ce3d41fa68f1cc46aec967182bc39b1a72f353d  0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
> > From b817065fb345c0075347235daed806ab8488e502 Mon Sep 17 00:00:00 2001
> > From: Stanislav Ionascu <stanislav.ionascu@gmail.com>
> > Date: Sun, 11 Aug 2019 21:10:30 +0200
> > Subject: [PATCH] avformat/matroskadec: properly parse stsd in v_quicktime
> >
> > Per matroska spec, v_quicktime contains the complete stsd atom, after
> > the mandatory size + fourcc. By properly parsing the hvcc sub-atoms of
> > the track, it becomes possible to demux/decode mp4/mov tracks stored as is
> > in matroska containers.
> >
> > QuickTime palette parsing is reused from the stsd parser results.
> > For non compliant input (when fourcc comes without the size), shift and
> > prepend the size to the private data.
> > Reading the SMI atom is reused from the stsd parser.
> >
> > Signed-off-by: Stanislav Ionascu <stanislav.ionascu@gmail.com>
>
> seems to break:
> ./ffmpeg -i tickets/3256/output-ffmpeg-20140109-git-c0a33c4.mkv -t 2 f.mp4
>
> https://samples.ffmpeg.org/ffmpeg-bugs/trac/ticket3256/output-ffmpeg-20140109-git-c0a33c4.mkv

Indeed. I looked at its private data and the fourcc code is 'SMI '.
Restored the SMI<->SVQ3 workaround.

Latest version attached.




> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Avoid a single point of failure, be that a person or equipment.
> _______________________________________________
> 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".

Patch hide | download patch | download mbox

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 4e20f15792..88bc89c545 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2473,25 +2473,46 @@  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;
+            AVIOContext *stsd_ctx = NULL;
+            void *priv_data;
+            int nb_streams;
             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);
+            av_log(matroska->ctx, AV_LOG_TRACE,
+                   "FourCC found %s.\n", av_fourcc2str(fourcc));
+            priv_data = st->priv_data;
+            nb_streams = s->nb_streams;
+            mc = av_mallocz(sizeof(*mc));
+            if (!mc)
+                return AVERROR(ENOMEM);
+            stsd_ctx = avio_alloc_context(track->codec_priv.data,
+                                track->codec_priv.size,
+                                0, NULL, NULL, NULL, NULL);
+            if (!stsd_ctx)
+                return AVERROR(ENOMEM);
+            mc->fc = s;
+            st->priv_data = msc = av_mallocz(sizeof(MOVStreamContext));
+            if (!msc) {
+                av_free(mc);
+                st->priv_data = priv_data;
+                return AVERROR(ENOMEM);
             }
-            if (codec_id == AV_CODEC_ID_NONE)
-                av_log(matroska->ctx, AV_LOG_ERROR,
-                       "mov FourCC not found %s.\n", av_fourcc2str(fourcc));
-            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)) {
-                    bit_depth &= 0x1F;
-                    track->has_palette = 1;
-                }
+            /* 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, stsd_ctx, 1);
+            av_free(msc);
+            av_free(mc);
+            avio_context_free(&stsd_ctx);
+            st->priv_data = priv_data;
+            s->nb_streams = nb_streams;
+
+            // dvh1 in mkv is likely HEVC
+            if (st->codecpar->codec_tag == MKTAG('d','v','h','1')) {
+                codec_id = AV_CODEC_ID_HEVC;
             }
         } else if (codec_id == AV_CODEC_ID_PCM_S16BE) {
             switch (track->audio.bitdepth) {