GNOME Bugzilla – Bug 654379
matroskamux: make default framerate optional per stream
Last modified: 2016-09-29 07:19:39 UTC
Created attachment 191681 [details] [review] patch v1 Current webmmuxer was not really changed to make valid stram. This patch disables every thing what webm do not support. I also disabled DefaultDuration, since webm should use only sipmeblock, there is no way to correct duration.
Review of attachment 191681 [details] [review]: ::: gst/matroska/matroska-mux.h @@ +88,3 @@ /* EBML DocType. */ const gchar *doctype; + gboolean is_webm; Why do you add a new field instead of changing the doctype to an enum and convert the enum to a string in the single place where doctype is used?
Review of attachment 191681 [details] [review]: ::: gst/matroska/matroska-mux.c @@ +2694,3 @@ if (collect_pad->track->type == GST_MATROSKA_TRACK_TYPE_VIDEO) { if (GST_BUFFER_FLAG_IS_SET (buf, GST_BUFFER_FLAG_DELTA_UNIT)) { + if (!mux->is_webm && (strcmp (collect_pad->track->codec_id, Why only for the !webm case? @@ +2708,3 @@ + } else if (collect_pad->track->type == GST_MATROSKA_TRACK_TYPE_AUDIO && + mux->is_webm) + flags = 0x80; Isn't this setting the discardable flag? You probably meant to set the keyframe flag
I'm not a big fan of applying this patch. The WebM spec for the container was written fairly quickly and has a number of known bugs that haven't been fixed. I'd prefer to wait until there's a spec that is consistent and makes sense before making the muxer pedantic. In particular, the bit about "You can't have anything beyond what we specified" is rather dumb, since that neatly excludes metadata, live streaming, and captioning. I could be convinced that a "pedantic=true" flag would be appropriate.
David, you have right. I reduced this patch to optional "default frame duration". Only this option make difference for other players. If default frame duration (frame rate) is written: mplayer - seek failed vlc - ok firefox - ok chromium - seek filed mkvmerge - converted frames with duration=0 to key frames (should be fixed now)
Created attachment 191847 [details] [review] patch v2
this patch depend on this one: https://bugzilla.gnome.org/show_bug.cgi?id=653080
As it was discussed on IRC, we should make it option per stream not per file.
Created attachment 198884 [details] [review] patch v3 New patch, this add PadOption for frmaerate.
Created attachment 200752 [details] [review] patch v4 Latest version of this patch
any updates?
commit 23594b0324e4bfdc153fcf89569b74bdb918c94d Author: Alexey Fisher <bug-track@fisher-privat.net> Date: Tue Oct 11 02:07:13 2011 +0200 matroskamux: make default framerate optional per stream there is at least two use cases where default frame rate should or may be disabled: - vp8 stream with altref frame enabled. If default frame rate is enabled, some players will missinterprete it (critical!) - for webm container, to reduce micro overhead - for stream with variable frame rate. Signed-off-by: Alexey Fisher <bug-track@fisher-privat.net>
Running suite(s): matroskamux Unexpected critical/warning: g_param_spec_internal: assertion `!(flags & G_PARAM_STATIC_NAME) || is_canonical (name)' failed Unexpected critical/warning: g_param_spec_internal: assertion `!(flags & G_PARAM_STATIC_NAME) || is_canonical (name)' failed Unexpected critical/warning: g_param_spec_internal: assertion `!(flags & G_PARAM_STATIC_NAME) || is_canonical (name)' failed Unexpected critical/warning: g_param_spec_internal: assertion `!(flags & G_PARAM_STATIC_NAME) || is_canonical (name)' failed 0%: Checks: 4, Failures: 4, Errors: 0 gstcheck.c:72:F:general:test_ebml_header:0: Unexpected critical/warning: g_param_spec_internal: assertion `!(flags & G_PARAM_STATIC_NAME) || is_canonical (name)' failed gstcheck.c:72:F:general:test_vorbis_header:0: Unexpected critical/warning: g_param_spec_internal: assertion `!(flags & G_PARAM_STATIC_NAME) || is_canonical (name)' failed gstcheck.c:72:F:general:test_block_group:0: Unexpected critical/warning: g_param_spec_internal: assertion `!(flags & G_PARAM_STATIC_NAME) || is_canonical (name)' failed gstcheck.c:72:F:general:test_reset:0: Unexpected critical/warning: g_param_spec_internal: assertion `!(flags & G_PARAM_STATIC_NAME) || is_canonical (name)' failed FAIL: elements/matroskamux commit 337533ee843eb86289f4550293b30a853b31e94e Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Thu Dec 1 13:22:42 2011 +0000 matroska-mux: fix name of new property and the unit test https://bugzilla.gnome.org/show_bug.cgi?id=654379
For future reference, calling a property "frame-duration" when it's a boolean is just awful (should have noticed that earlier when I changed the name..)
The latest version of the WebM spec says that DefaultDuration is supported: http://www.webmproject.org/docs/container/ Going to enable it by default again now, so that we can guess the framerate from it. See also bug #772141 Might want to make the property deprecated...