After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 740130 - matroskamux: wrong duration on some files
matroskamux: wrong duration on some files
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.4.4
Other Linux
: Normal blocker
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 734424
Blocks:
 
 
Reported: 2014-11-14 17:04 UTC by Nicola
Modified: 2015-08-11 21:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
file that show the initial problem (931.80 KB, application/octet-stream)
2014-11-14 23:39 UTC, Nicola
  Details
possible fix (1.02 KB, patch)
2014-11-18 15:59 UTC, Nicola
committed Details | Review
discoverer: let videorate compute a framerate when none provided (5.15 KB, patch)
2015-05-14 13:28 UTC, Thibault Saunier
reviewed Details | Review

Description Nicola 2014-11-14 17:04:25 UTC
please take a look at the file here:

http://195.250.34.59/temp/wrong_duration.mkv

this file is without indexes

if you try to add indexes with a pipeline like this:

filesrc ! matroskademux ! matroskamux ! filesink

you'll get a file with a duration header of about 6 minutes 

the buffer timestamp indicate that the file is 5 minutes,

mkvmerge -o out.mkv wrong_duration.mkv

produce a file with a 5 minutes duration header, this is correct

maybe gstreamer matroskamux get confused from timestamp that occasionally go backwards, for example:

0:05:00.098000000
0:05:00.065000000
Comment 1 Nicola 2014-11-14 17:25:11 UTC
I think the problem is here:

http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/matroska/matroska-mux.c#n3324

the input file has 9025 frames and each frame has a duration setted to 0:00:00.040000000 but the real duration of each buffer is less than 40 milliseconds. I guess the problem arise since some gstreamer plugin assume 25 fps somewhere, for example matroskamux, and write an invalid header for file such this with 30 fps, suggestions?
Comment 2 Nicola 2014-11-14 17:59:45 UTC
a quick (workaround?) is not set duration at all in matroskamux, probably is the best solution here: better a missing information than a wrong one
Comment 3 Nicola 2014-11-14 23:39:39 UTC
Created attachment 290744 [details]
file that show the initial problem

The initial problem can be see with the attached file:

gst-launch-1.0 -v filesrc location=/tmp/test.gdp ! gdpdepay ! fakesink

show:

/GstPipeline:pipeline0/GstFakeSink:fakesink0.GstPad:sink: caps = "video/x-h264\,\ stream-format\=\(string\)avc\,\ alignment\=\(string\)au\,\ codec_data\=\(buffer\)014d401effe1000f674d401e9a6604012d80b50501050201000468ee3c80\,\ width\=\(int\)512\,\ height\=\(int\)288\,\ framerate\=\(fraction\)0/1\,\ parsed\=\(boolean\)true\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ level\=\(string\)3\,\ profile\=\(string\)main"

so framerate is 0/1

now:

gst-launch-1.0 -v filesrc location=/tmp/test.gdp ! gdpdepay ! matroskamux ! filesink location=/tmp/test.mkv

and finally:

gst-launch-1.0 -v filesrc location=/tmp/test.mkv ! matroskademux streamable=true ! fakesink

show:

/GstPipeline:pipeline0/GstFakeSink:fakesink0.GstPad:sink: caps = "video/x-h264\,\ level\=\(string\)3\,\ profile\=\(string\)main\,\ codec_data\=\(buffer\)014d401effe1000f674d401e9a6604012d80b50501050201000468ee3c80\,\ stream-format\=\(string\)avc\,\ alignment\=\(string\)au\,\ width\=\(int\)512\,\ height\=\(int\)288\,\ framerate\=\(fraction\)25/1"

so the framerate is now 25/1 and this cause a wrong 40 milliseconds duration and so a wrong duration in matroska header if you remux to add indexes
Comment 4 Nicola 2014-11-18 15:48:21 UTC
I did some more analysis, the problem is generated here:

http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/matroska/matroska-demux.c#n5079

so the demuxer hardcode 25 fps:

/* sort of a hack to get most codecs to support,
 * even if the default_duration is missing */

this cause problem if the stream has more than 25 fps and no duration is setted.

Using something like this

gst_structure_set (structure, "framerate", GST_TYPE_FRACTION, 0, 1, NULL);

seems better and it works for my use case, what do you think about?  

thanks
Comment 5 Nicola 2014-11-18 15:59:55 UTC
Created attachment 290924 [details] [review]
possible fix

this patch set framerate as 0/1 when duration is not known, this should be the right fix, if you want I can preserve the hack and set framerate as 30/1 or 60/1 this will cause a wrong frame duration, however there is already some logic inside the muxer to handle with that (in the case the file is remuxed)

http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/gst/matroska/matroska-mux.c#n2270
Comment 6 Nicola 2014-12-02 01:08:30 UTC
can this patch be reviewed for 1.6 cycle? thanks
Comment 7 Sebastian Dröge (slomo) 2014-12-04 17:21:51 UTC
This is probably a leftover from the times when unknown framerates were a real problem.

commit c466ff474859cce91e9dd9b906c48f5fe0c4b164
Author: Nicola Murino <nicola.murino@gmail.com>
Date:   Tue Nov 18 16:52:40 2014 +0100

    matroskademux: set framerate 0/1 when duration is not known
    
    https://bugzilla.gnome.org/show_bug.cgi?id=740130
Comment 8 Thibault Saunier 2014-12-08 16:43:30 UTC
That commit broke this test: https://ci.gstreamer.net/job/GStreamer-master-validate/122/testReport/validate.file.transcode/to_mp3_and_h264_in_mp4/vorbis_vp8_0_webm/

So basically you can reproduce with:

gst-validate-launcher --sync -t validate.file.transcode.to_mp3_and_h264_in_mp4.vorbis_vp8_0_webm

Reopening the bug and putting it as BLOCKER as it is a regression.
Comment 9 Thibault Saunier 2014-12-09 11:49:46 UTC
Setting #734424 blocking that bug because the regression introduced in that commit is actually a result of that bug (which we were not noticing previously because we were exposing a fake framerate).
Comment 10 Sebastian Dröge (slomo) 2015-03-15 15:21:36 UTC
So this here is fixed once videorate is fixed?
Comment 11 Thibault Saunier 2015-03-15 16:46:35 UTC
We also need another patch in the discoverer so that it plug videorate.

I pushed it here: http://phabricator.freedesktop.org/D37

I am not 100% we want to go this way, so please let me know.

(let me know if you prefer I attach it here)
Comment 12 Tim-Philipp Müller 2015-05-10 22:53:05 UTC
> (let me know if you prefer I attach it here)

Yes, please don't put patches from the main modules into phabricator until we've migrated, and attach it here.
Comment 13 Tim-Philipp Müller 2015-05-10 23:05:09 UTC
So what exactly is it that makes the validate test fail?
Comment 14 Thibault Saunier 2015-05-14 13:28:47 UTC
Created attachment 303363 [details] [review]
discoverer: let videorate compute a framerate when none provided

Summary:
And make sure that the updated values are properly set on the main
GstDiscovererStreamInfo caps value (and not only its structure values)
Comment 15 Tim-Philipp Müller 2015-08-10 15:28:36 UTC
Comment on attachment 303363 [details] [review]
discoverer: let videorate compute a framerate when none provided

I don't think this is the right approach. If we want to make up framerates, it'd probably be better to let the demuxer make them up, because it has more and better information.

It's also not clear to me at all that (a) making up / guesstimating a framerate is better than signalling that there is no fixed framerate when there is none, and (b) that this is actually a blocker
Comment 16 Thibault Saunier 2015-08-10 16:53:38 UTC
I do not really care tbh, I guess we can just WONTFIX that bug as current behaviour is not that bad (signalling that there is no fixed framerate).
Comment 17 Tim-Philipp Müller 2015-08-10 22:14:33 UTC
Ok, great, thanks.

Just to make sure I understood the situation correctly, here is my understanding:

- before matroskademux used to just say framerate=25/1 when we didn't know
- that was changed to say framerate=0/1 (variable framerate) when we didn't know
- validate tests broke because they were expecting 25/1
- now it might be desirable to investigate if we can advertise a better/proper framerate instead of 0/1, but 0/1 is not worse than a completely made up 25/1, so we may just as well update the validate tests and leave the task to figure out something better for some other time.
Comment 18 Nicolas Dufresne (ndufresne) 2015-08-11 13:03:09 UTC
We need to update the media file then.

https://ci.gstreamer.net/job/GStreamer-master-validate/lastCompletedBuild/testReport/validate.http/media_check/vorbis_vp8_1_webm/
Comment 19 Nicolas Dufresne (ndufresne) 2015-08-11 13:15:18 UTC
commit bcb2afbf221abbae3a479cb55185a28fb369492d
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Tue Aug 11 09:13:15 2015 -0400

    media: MKV framerate use to be a random value
    
    It's now properly set to 0/1, letting GStreamer entirely rely on
    timestamps as it should.
Comment 20 Thibault Saunier 2015-08-11 21:31:31 UTC
Thanks Nicolas, we need a gst-validate-integration-testsuites product/project in the bug tracker actually :)