Hi, thanks for looking into this.
On Sun, Aug 18, 2019 at 4:55 AM Andreas Rheinhardt <andreas.rheinha...@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-requ...@ffmpeg.org with subject "unsubscribe".
0001-avformat-matroskadec-properly-parse-stsd-in-v_quickt.patch
Description: Binary data
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".