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

[wm4]

On Fri, 3 Feb 2017 10:04:34 -0800
Vignesh Venkatasubramanian <vigneshv-at-google.com at ffmpeg.org> wrote:

> On Thu, Feb 2, 2017 at 10:16 PM, wm4 <nfxjfg at googlemail.com> wrote:
> > On Thu, 2 Feb 2017 10:47:52 -0800
> > Vignesh Venkatasubramanian <vigneshv-at-google.com at ffmpeg.org> wrote:
> >  
> >> 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).  
> >
> > Yeah, the current spec just says "Timestamp of another frame used as a
> > reference (ie: B or P frame). The timestamp is relative to the  block
> > it's attached to."
> >
> > Is this a bug? Did FFmpeg always mux this incorrectly?
> >  
> 
> Technically it is a bug. Yes ffmpeg has always muxed it this way. But
> see the reply below.
> 
> > Is there even an implementation that uses the value written to block
> > reference elements? (And not in a trivial way like FFmpeg.)  
> 
> AFAIK, there is no practical use for the value written into
> BlockReference. All the WebM codecs (vp8, vp9, vorbis, opus) have the
> reference frame information in their bitstream and do not care about
> what is specified in the container. In fact, in case of VP9 there can
> be multiple reference frames and i'm not sure if there's even a way to
> specify that in the container. So far, the only use of this element
> has been to determine whether or not the Block is a keyframe.

Yes, that seems to be pretty much anyone's opinion about this
(including my own).

Still feels weird that FFmpeg is writing essentially arbitrary values.