[FFmpeg-devel] [PATCH] lavf/matroskadec: fix is_keyframe for early Blocks

[Vignesh Venkatasubramanian]

On Tue, Jan 31, 2017 at 10:18 PM, wm4 <nfxjfg at googlemail.com> wrote:
>
> On Tue, 31 Jan 2017 12:02:01 -0800
> Chris Cunningham <chcunningham at chromium.org> wrote:
>
> > Thanks for taking a look.
> >
> > Definitely missing a "break;" - will fix in subsequent patch.
> >
> > Agree timestamps should be relative (didn't realize this). Vignesh points
> > out that "0" in the test file is due to a bug in ffmpeg (and probably other
> > muxers) where this value is not written as a relative timestamp, but
> > instead as the timestamp of the previous frame. https://github.com/FFmp
> > eg/FFmpeg/blob/master/libavformat/matroskaenc.c#L2053
> > <https://www.google.com/url?q=https%3A%2F%2Fgithub.com%2FFFmpeg%2FFFmpeg%2Fblob%2Fmaster%2Flibavformat%2Fmatroskaenc.c%23L2053&sa=D&sntz=1&usg=AFQjCNGs8m6GsWbhTvCZl0Q_juGAldQblA>
>
> Just a few lines below this reads
>
>    mkv->last_track_timestamp[track_number - 1] = ts - mkv->cluster_pts;
>
> which looks like it intends to write a relative value. Though "ts" can
> be a DTS, while the other value is always a PTS.

Just to clarify: This line makes the timestamp relative to the
cluster's timestamp. Not relative to the block its referencing (which
is what the spec says if i understand it correctly).

>
> > Anyway, I'm all for following Haali. Its not obvious how best to do this. I
> > don't think there's any great default value to indicate "not-set" and the
> > generic embl parsing code that reads the reference timestamp doesn't really
> > lend itself to setting an additional field like
> > MatroskaBlock.has_reference. I can sort this out, but I'll pause in case
> > you have a recommendation in-mind.
>
> I don't know a nice way either. Here are 3 potential ways I came up
> with:
>
> 1. Use a better dummy value, like INT64_MIN, which is almost 100%
>    unlikely to appear in a valid file. (This is probably equivalent to
>    your intention in original patch.)
> 2. Add a new "presence_flag_offset" field to EbmlSyntax - the EBML
>    reader will write a 1 to it if the element was found. (This is a bit
>    annoying, because 0 is a valid offset, and you surely wouldn't want
>    to change all the other element definitions.)
> 3. Add a new type like EBML_PRESENCE, which writes whether the element
>    was present or not to data_offset, instead of the element's content
>    or its default value. (There are some other special "types" like
>    EBML_STOP, so maybe this idea isn't all too hacky.)
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel




-- 
Vignesh