[FFmpeg-devel] [PATCH 1/1] avformat/matroska: fully parse stsd atom in v_quicktime tracks
Stanislav Ionascu
stanislav.ionascu at gmail.com
Thu Aug 22 08:59:33 EEST 2019
Hi!
On Tue, Aug 20, 2019 at 10:19 AM Michael Niedermayer
<michael at 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 at gmail.com> wrote:
> > >
> > > Hello,
> > >
> > > I am no expert on mov (and so this should definitely be looked at from
> > > someone who is), but I have some points:
> > >
> > > Stanislav Ionascu:
> > > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > > > index 1ea9b807e6..2a397c909a 100644
> > > > --- a/libavformat/matroskadec.c
> > > > +++ b/libavformat/matroskadec.c
> > > > @@ -2473,25 +2473,58 @@ static int
> > > matroska_parse_tracks(AVFormatContext *s)
> > > > } else if (!strcmp(track->codec_id, "V_QUICKTIME") &&
> > > > (track->codec_priv.size >= 21) &&
> > > > (track->codec_priv.data)) {
> > > > + MOVStreamContext *msc;
> > > > + MOVContext *mc = NULL;
> > > > + void *priv_data;
> > > > + int nb_streams;
> > >
> > > You could initialize nb_streams and priv_data here. And the
> > > initialization of mc is unnecessary.
> >
> > Done.
> >
> > >
> > > > int ret = get_qt_codec(track, &fourcc, &codec_id);
> > > > if (ret < 0)
> > > > return ret;
> > > > - if (codec_id == AV_CODEC_ID_NONE &&
> > > AV_RL32(track->codec_priv.data+4) == AV_RL32("SMI ")) {
> > > > - fourcc = MKTAG('S','V','Q','3');
> > > > - codec_id = ff_codec_get_id(ff_codec_movvideo_tags,
> > > fourcc);
> > > > + mc = av_mallocz(sizeof(*mc));
> > > > + if (!mc)
> > > > + return AVERROR(ENOMEM);
> > > > + priv_data = st->priv_data;
> > > > + nb_streams = s->nb_streams;
> > > > + mc->fc = s;
> > > > + st->priv_data = msc = av_mallocz(sizeof(MOVStreamContext));
> > > > + if (!msc) {
> > > > + av_free(mc);
> > > > + st->priv_data = priv_data;
> > > > + return AVERROR(ENOMEM);
> > > > }
> > > > + ffio_init_context(&b, track->codec_priv.data,
> > > > + track->codec_priv.size,
> > > > + 0, NULL, NULL, NULL, NULL);
> > > > +
> > > > + /* ff_mov_read_stsd_entries updates stream s->nb_streams-1,
> > > > + * so set it temporarily to indicate which stream to
> > > update. */
> > > > + s->nb_streams = st->index + 1;
> > > > + ff_mov_read_stsd_entries(mc, &b, 1);
> > >
> > > Is it intentional that you don't check the return value here?.
> >
> > Added the check.
> >
> > >
> > > > +
> > > > + /* copy palette from MOVStreamContext */
> > > > + track->has_palette = msc->has_palette;
> > > > + if (track->has_palette) {
> > > > + /* leave bit_depth = -1, to reuse
> > > bits_per_coded_sample */
> > > > + memcpy(track->palette, msc->palette, AVPALETTE_COUNT);
> > >
> > > In case the track has a palette, your patch would only copy 256 bytes
> > > of it, but a palette consists of 256 uint32_t. (This link
> > > https://drive.google.com/drive/folders/0B3_pEBoLs0faWElmM2FnLTZYNlk
> > > from the commit message that added the palette stuff seems to still
> > > work if you need samples for this.)
> >
> > Indeed. I've corrected that. Also remuxed all mov's into mkv's, and
> > verified that they all still work.
> >
> > >
> > > > + }
> > > > +
> > > > + av_free(msc);
> > > > + av_free(mc);
> > > > + st->priv_data = priv_data;
> > > > + s->nb_streams = nb_streams;
> > > > + fourcc = st->codecpar->codec_tag;
> > > > + codec_id = st->codecpar->codec_id;
> > > > +
> > > > + av_log(matroska->ctx, AV_LOG_TRACE,
> > > > + "mov FourCC found %s.\n", av_fourcc2str(fourcc));
> > > > +
> > > > if (codec_id == AV_CODEC_ID_NONE)
> > > > av_log(matroska->ctx, AV_LOG_ERROR,
> > > > - "mov FourCC not found %s.\n",
> > > av_fourcc2str(fourcc));
> > >
> > > If the codec_id turns out to be AV_CODEC_ID_NONE, two strings will be
> > > output (at least on loglevel trace): "mov FourCC found %s.\n" and then
> > > "mov FourCC not found %s.\n". The first of these is superfluous in
> > > this case.
> >
> > Removed it, as stsd parser will also trace the FourCC code.
> >
> > >
> > > > - if (track->codec_priv.size >= 86) {
> > > > - bit_depth = AV_RB16(track->codec_priv.data + 82);
> > > > - ffio_init_context(&b, track->codec_priv.data,
> > > > - track->codec_priv.size,
> > > > - 0, NULL, NULL, NULL, NULL);
> > > > - if (ff_get_qtpalette(codec_id, &b, track->palette)) {
> > >
> > > If I am not extremely mistaken, then there is no need to include
> > > qtpalette.h any more after removing this function call.
> >
> > Yes. Removed as it's not necessary.
> >
> > >
> > > > - bit_depth &= 0x1F;
> > > > - track->has_palette = 1;
> > > > - }
> > > > + "mov FourCC not found %s.\n",
> > > av_fourcc2str(fourcc));
> > > > +
> > > > + // dvh1 in mkv is likely HEVC
> > > > + if (fourcc == MKTAG('d','v','h','1')) {
> > > > + codec_id = AV_CODEC_ID_HEVC;
> > >
> > > Your changes for dvh1 should probably be moved to a separate commit.
> >
> > Removed. The stsd parser takes care of that. It looks for the hvcC atom, and
> > correctly marks the stream as HEVC if it is found.
> >
> > I've uploaded a sample via vlc uploader (as
> > unplayable_dolby_vision_v_quicktime_track),
> > not sure where it ended up.
> >
> > Latest patch attached.
> >
> > >
> > > > }
> > > > } else if (codec_id == AV_CODEC_ID_PCM_S16BE) {
> > > > switch (track->audio.bitdepth) {
> > >
> > > - Andreas
> >
> > Thanks!
> > Stan.
> >
> >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel at ffmpeg.org
> > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > > To unsubscribe, visit link above, or email
> > > ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
> > 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 at 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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
Type: application/octet-stream
Size: 4518 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190822/8b91e7f3/attachment.obj>
More information about the ffmpeg-devel
mailing list