| Message ID | 20190813191620.14480-2-stanislav.ionascu@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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".
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 [...]
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".
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 [...]
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".
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
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".
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 [...]
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".
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) {
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(-)