[FFmpeg-devel] [PATCH 1/1] avformat/matroska: fully parse stsd atom in v_quicktime tracks
Michael Niedermayer
michael at niedermayer.cc
Tue Aug 20 11:19:15 EEST 2019
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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Avoid a single point of failure, be that a person or equipment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190820/f89ec53c/attachment.sig>
More information about the ffmpeg-devel
mailing list