GNOME Bugzilla – Bug 745192
matroskademux: V_MS-VFW-FOURCC streams have DTS instead of PTS
Last modified: 2015-03-16 11:40:12 UTC
Created attachment 297942 [details] [review] Patch When such stream is present demuxer should set DTS on buffers instead of PTS. This is consistent with how VLC and libav/ffmpeg handle VFW streams. Sample file https://s3.amazonaws.com/MatejK/Samples/Matroska-VFW-DTS-Only.mkv
commit fa283f407f3de4a31ecf756f6cdd02aeb9d162d3 Author: Matej Knopp <matej.knopp@gmail.com> Date: Thu Feb 26 02:12:18 2015 +0100 matroskademux: V_MS/VFW/FOURCC streams have DTS instead of PTS When such stream is present demuxer should set DTS on buffers instead of PTS. This is consistent with how VLC and libav/ffmpeg handle VFW streams. Sample file https://s3.amazonaws.com/MatejK/Samples/Matroska-VFW-DTS-Only.mkv https://bugzilla.gnome.org/show_bug.cgi?id=745192
this patch break even more transmuxing, a simple pipeline like this: matroskademux ! matroskamux does not work anymore with the above file since all pts are not setted
You need a parser between the two, and ideally that parser would calculate DTS from PTS and the other way around. You already had the same problem with avidemux ! matroskamux for example.
ideally yes, but actually parser seems unable to do this job
Well, I guess the muxer should do the same thing. When muxing VFW it should use DTS instead of PTS, as that's what demuxers expect anyway. As for the timestamps, the MPEG4 parser could compute PTS from DTS, that shouldn't be too difficult. For H.264 and VC1 it would be more complicated.
(In reply to Matej Knopp from comment #5) > Well, I guess the muxer should do the same thing. When muxing VFW it should > use DTS instead of PTS, as that's what demuxers expect anyway. this more or less the workaround that I use in my application, I demux with something like demuxer ! appsink and when I receive the buffer I set pts from dts if pts is not setted, can something similar be used as fallback in muxers until the parsers are fixed? > > As for the timestamps, the MPEG4 parser could compute PTS from DTS, that > shouldn't be too difficult. For H.264 and VC1 it would be more complicated.
You can't just use DTS instead of PTS for streams with bframes when there should be PTS in the output. Such file will not be compliant. It will play in some players but will have problems in other (that use decoders that don't output frames in presentation order and rely on PTS, such as Apple decoders) The proper way in this case would be to mux DTS instead of PTS for VFW streams. IIRC avidemux already sets DTS (although it seems to set PTS on keyframes as well, which I'm not sure is right; and avimux always muxes PTS which definitely isn't right). It seems that right now avidemux | matroskamux and avidemux | avimux wouldn't work either.
Matej, thanks for your explaination, I'll try to make a patch for matroskamux so that it use dts and not pts for V_MS/VFW/FOURCC streams, this should be fine since for this stream pts is supposed to be not setted
Created attachment 298035 [details] [review] matroskamux: store DTS for V_MS/VFW/FOURCC streams this store dts for the following mimetypes: - video/x-huffyuv - video/x-divx - video/x-dv - video/x-h263 - video/x-msmpeg - video/x-wmv - image/jpeg
Created attachment 298039 [details] [review] initialize dts_only
commit e676b8ba9c1b5969611aab2f4e9ab3742968f1b0 Author: Nicola Murino <nicola.murino@gmail.com> Date: Thu Feb 26 23:41:47 2015 +0100 matroskamux/demux: initialize dts_only https://bugzilla.gnome.org/show_bug.cgi?id=745192 commit 09b8f0efc30368517e839f1ccb284592683321cb Author: Nicola Murino <nicola.murino@gmail.com> Date: Thu Feb 26 23:28:11 2015 +0100 matroskamux: store DTS for V_MS/VFW/FOURCC streams https://bugzilla.gnome.org/show_bug.cgi?id=745192
the muxer patch is not completly correct, in matroskamux there are calls like this: GST_BUFFER_TIMESTAMP (buf) in a many places (for example for duration calculation), I think an helper method should be done to return the pts/dts, I'll try to work on this the next week if noone is faster than me. Also the demuxer use GST_BUFFER_TIMESTAMP in a log message, this could be make debug harder
Created attachment 298106 [details] [review] proposed fix
Created attachment 298107 [details] [review] use helper method in demux too
Also an interesting question from IRC: <drakkan1000> slomo, hi, I have a question about this commit http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=fa283f407f3de4a31ecf756f6cdd02aeb9d162d3 now that the demuxer can output only dts, how to handle for example gap events? In gap events the timestamp is the pts, but now we can have no pts
Comment on attachment 298106 [details] [review] proposed fix Please give this helper method a proper namespace prefix, otherwise this can easily conflict with other code especially during static linking. E.g. call it gst_matroska_track_get_buffer_timestamp()
Created attachment 298269 [details] [review] updated patch
(In reply to Sebastian Dröge (slomo) from comment #15) > Also an interesting question from IRC: > > <drakkan1000> slomo, hi, I have a question about this commit > http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/ > ?id=fa283f407f3de4a31ecf756f6cdd02aeb9d162d3 now that the demuxer can output > only dts, how to handle for example gap events? In gap events the timestamp > is the pts, but now we can have no pts for matroska timestamp in gap and new segment events should be dts or pts based on the stream type after applying these patches
so probably the gst_event_parse_gap doc should be fixed
Created attachment 298389 [details] [review] update collected pad duration only for buffers with different timestamp after the latest patch avidemux ! matroskamux works for dts only file (for example divx inside avi) but produce wrong duration for some files because collect pad duration result greater than real duration. This patch solve also this issue, here is a log that show the duplicated ts for different buffers /GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain ******* (fakesink0:sink) (4965 bytes, dts: 0:01:05.222222100, pts: none, duration: 0:00:00.039989100, offset: 16690594, offset_end: -1, flags: 00002000 delta-unit ) 0x7fd0d80034c0 /GstPipeline:pipeline0/GstFakeSink:fakesink0: last-message = chain ******* (fakesink0:sink) (8 bytes, dts: 0:01:05.222222100, pts: none, duration: 0:00:00.039989100, offset: 16695559, offset_end: -1, flags: 00002000 delta-unit ) 0x7fd0d80035d0 the patch can help in general and collectad duration should be updated from buffer duration only if the buffer timestamps are not equal
Review of attachment 298269 [details] [review]: Looks good, but please do the following cleanup :) Then I'll merge it ::: gst/matroska/matroska-mux.c @@ +3411,3 @@ GST_LOG_OBJECT (mux, "have video keyframe, ts=%" GST_TIME_FORMAT, + GST_TIME_ARGS (gst_matroska_track_get_buffer_timestamp + (collect_pad->track, buf))); Instead of calling the function many, many times here... just store the timestamp in some local variable :) Will make it easier to read the code.
Review of attachment 298389 [details] [review]: ::: gst/matroska/matroska-mux.c @@ +3480,3 @@ + || gst_matroska_track_get_buffer_timestamp (collect_pad->track, + buf) != collect_pad->last_collected_duration_ts) { + collect_pad->duration += GST_BUFFER_DURATION (buf); I think the duration should rather be the difference between the first buffer's timestamp and the last buffer's timestamp+duration. Counting it here is not going to give useful results and accumulates rounding errors.
Created attachment 298462 [details] [review] updated patch
Created attachment 298463 [details] [review] remove duration accumulation logic
commit c4e542de695e07cab64f390b4523b9b68ba84971 Author: Nicola Murino <nicola.murino@gmail.com> Date: Tue Mar 3 19:19:50 2015 +0100 matroskamux: Remove duration accumulation logic Duration accumulation can cause rounding errors and generate wrong duration with different buffers that share the same timestamp. https://bugzilla.gnome.org/show_bug.cgi?id=745192 commit f727762c1f186c15d6e656e92c01306796de8692 Author: Nicola Murino <nicola.murino@gmail.com> Date: Tue Mar 3 18:40:16 2015 +0100 matroska: Add an helper method to get buffer timestamps ... and replace GST_BUFFER_TIMESTAMP that always return PTS with this method that return PTS or DTS based on stream type. https://bugzilla.gnome.org/show_bug.cgi?id=745192
Created attachment 298785 [details] [review] set pts for keyframes in any case this should be the last missing place, this way the behaviour is the same as avidemux and for keyframe pts and dts match in any case
the latest patch fix playback for some jpeg file for example this one: http://195.250.34.59/temp/jpeg.mkv jpeg inside matroska is now dts only, a buffer with dts and not pts seems to create problems to jpegdec. I don't know if the bug is in jpegdec, however avidemux put pts on keyframe too
I'm not sure if this is correct. IIRC there are situations where the DTS and PTS of keyframes can be different too. Maybe instead there should be a keyframe-only setting, which is then used for JPEG and similar formats?
Indeed, for at least H264, DTS can be smaller then PTS at keyframes. This figure explains a bit that shift: https://software.intel.com/sites/default/files/pts-dts_shift_explain.gif https://software.intel.com/en-us/forums/topic/501639
Created attachment 298803 [details] [review] tentative patch added a new context property "is_raw", this property is setted to true for jpeg and raw formats for VFW case (that is the only one that really matter) we can have raw and not raw formats For file muxed by gstreamer raw formats should be only jpeg and huffyuv (not sure if dv is raw). The patch handle these codec I don't know if a matroska file muxed by app different from gstreamer can store other raw formats for VFW streams (probably yes), in this case is there a smarter way than the one in the patch to understand if a stream is raw or not without replicate all gst_riff_create_video_caps code?
(In reply to Nicola from comment #30) > Created attachment 298803 [details] [review] [review] > tentative patch > > added a new context property "is_raw", this property is setted to true for > jpeg and raw formats > > for VFW case (that is the only one that really matter) we can have raw and > not raw formats > > For file muxed by gstreamer raw formats should be only jpeg and huffyuv (not > sure if dv is raw). The patch handle these codec > > I don't know if a matroska file muxed by app different from gstreamer can > store other raw formats for VFW streams (probably yes), in this case is > there a smarter way than the one in the patch to understand if a stream is > raw or not without replicate all gst_riff_create_video_caps code? Unfortunately not, but the list of caps that are intra-only shouldn't be too big. (DV is included there btw) I would call that field intra_only instead of raw btw, that's more accurate.
Created attachment 298804 [details] [review] tentative patch v2 I added a list of formats that will be likely find in mkv file based on the entries in gst_riff_create_video_caps and the ones in matroska-ids.h, however the list should be easily extensible
Avidemux can probably get away with putting PTS=DTS on keyframe because no one sane muxes H.264 in avi anyway and I don't think mpeg 4 video can have open gop. I still think though that demuxer should only put PTS=DTS for streams that don't have reordering (and in that case do it on every frame). And if does set PTS=DTS on keyframes for reordered stream it better knows what it is doing :)
the latest patch set dts=pts on every frame for intra only stream
Review of attachment 298804 [details] [review]: ::: gst/matroska/matroska-demux.c @@ +4888,3 @@ + gst_caps_new_empty_simple ("video/x-ffv")); + for (iterator = intra_caps; iterator != NULL; iterator = iterator->next) { + if (gst_caps_can_intersect ((GstCaps *) iterator->data, caps)) { Why so complicated? static GstStaticCaps intra_caps = GST_STATIC_CAPS("image/jpeg; video/x-raw; ....."); context->intra_only = gst_caps_can_intersect (gst_static_caps_get (&intra_caps), caps);
Created attachment 299131 [details] [review] tentative patch v3 please review, thanks Nicola
commit bb3d82ef04107310e685db0bb0ddcc218dda1721 Author: Nicola Murino <nicola.murino@gmail.com> Date: Wed Mar 11 21:25:40 2015 +0100 matroskademux: for dts only stream set pts=dts for intra only formats https://bugzilla.gnome.org/show_bug.cgi?id=745192
As noted in bug 733720 I think the intra-only formats should not have pts=dts but dts=none, as e.g. videorate does not look at or touch dts at all, leading to trouble later if dts is preferred to pts (as sinks do, for example).
Jan, shouldn't decoder have taken care of that? Once you decode the stream you should have valid PTS on output and no DTS. You don't feed videorate raw stream.
Matroska can transport raw streams, though.
To clarify, yes, videorate wants video/x-raw streams (as well as image/jpeg or image/png). I'm using Matroska with raw streams for transport between processes.
actually matroskademux set pts = dts for intra only VFW streams. Before Matej' patch only pts was setted and never dts for all streams. We can easily modify mastroksademux to set pts and not dts for intra only VFW streams, only we need to understand if we agree on this