GNOME Bugzilla – Bug 754696
matroskamux: audio-only streams have all buffers flagged as delta units, causing problems with tcpserversink/multifdsink
Last modified: 2017-07-18 06:58:07 UTC
please try a pipeline like this one: gst-launch-1.0 -v pulsesrc ! audioconvert ! audioresample ! queue ! mulawenc ! matroskamux streamable=true ! fakesink silent=false you can observe that each buffer has delta-unit flag set, I think this is not correct, probably muxers should unset delta-unit on each audio buffer, what do you think about?
How or why does this affect you? It's done this way for streamable formats so that other elements can know where in the stream the start of a GOP if the stream has audio and video. multifdsink/tcpserversink for example can then keep a backbuffer and 'burst' newly-connecting clients with data from the start of the last GOP, then they don't have to wait for the next keyframe to start showing video. multifilesink makes use of it in some modes to make sure that a new file always starts with a GOP.
in my app I need to find keyframes, for audio only stream I have an hack since the standard method (used in multifdsink/tcpserversink) does not work, you can easily reproduce with this pipeline: gst-launch-1.0 -v pulsesrc ! audioconvert ! audioresample ! queue ! mulawenc ! matroskamux streamable=true ! tcpserversink port=3000 sync-method=1 gst-launch-1.0 -v tcpclientsrc port=3000 ! matroskademux ! fakesink sync=false silent=false it works if you use sync-method=0 on the first pipeline I think the muxer should properly mark audio buffers as keyframe unsetting the delta unit flag
You suggest the muxer should do this unconditionally? or only if there's no video stream?
(In reply to Tim-Philipp Müller from comment #3) > You suggest the muxer should do this unconditionally? or only if there's no > video stream? probably yes, I would like to hear your opinions to not produce an unacceptable patch. The bug seems to affect matroskamux only (streamable true/false make no difference), other muxers set no delta unit flag (avimux, flvmux)
Created attachment 310882 [details] [review] patch This patch fix the tcpserversink pipeline and my app, seems to not introduce regression for video only and audio+video streams too, please review
the patch assume that for audio only stream each buffer is managed as a video keyframe
any chance to review this patch before 1.6 release? This is not a regression, is broken in the same way even in 0.10, however actually matroska seems unable to properly mux audio only streams
ping
Comment on attachment 310882 [details] [review] patch I'm fine with the patch in principle, I'm just wondering about some specifics: >@@ -3599,7 +3600,7 @@ gst_matroska_mux_write_data (GstMatroskaMux * mux, GstMatroskaPad * collect_pad, > > if (mux->doctype_version > 1 && !write_duration) { >- if (is_video_keyframe) >+ if (is_video_keyframe || is_audio_only) > flags |= 0x80; > Does this mean we will set the keyframe flag on the simpleblock header. Not sure if that should ever be set for audio? I thought the goal was to unset the DELTA_UNIT flag on the buffers, so tcpserversink works nicely with audio-only streams? I also wonder if the line below should not rather have gst_ebml_write_flush_cache (ebml, is_video_keyframe || is_audio_only, buffer_timestamp); ?
(In reply to Tim-Philipp Müller from comment #9) > Comment on attachment 310882 [details] [review] [review] > patch > > I'm fine with the patch in principle, I'm just wondering about some > specifics: > > >@@ -3599,7 +3600,7 @@ gst_matroska_mux_write_data (GstMatroskaMux * mux, GstMatroskaPad * collect_pad, > > > > if (mux->doctype_version > 1 && !write_duration) { > >- if (is_video_keyframe) > >+ if (is_video_keyframe || is_audio_only) > > flags |= 0x80; > > > > Does this mean we will set the keyframe flag on the simpleblock header. Not > sure if that should ever be set for audio? > > I thought the goal was to unset the DELTA_UNIT flag on the buffers, so > tcpserversink works nicely with audio-only streams? setting the keyframe flag on the simpleblock header should let tcpserver to send starting from it, for what I understand seems fine, am I wrong? > > I also wonder if the line below should not rather have > > gst_ebml_write_flush_cache (ebml, is_video_keyframe || is_audio_only, > buffer_timestamp); probably yes, however this was buggy even before this patch so probably should be a separate commit, what do you think about? > > ?
> > Does this mean we will set the keyframe flag on the simpleblock header. Not > > sure if that should ever be set for audio? > > > > I thought the goal was to unset the DELTA_UNIT flag on the buffers, so > > tcpserversink works nicely with audio-only streams? > > setting the keyframe flag on the simpleblock header should let tcpserver to > send starting from it, for what I understand seems fine, am I wrong? What I meant is that what the patch does it two things: a) gstreamer buffer flagged as keyframe on the GstBuffer, b) the audio data in the matroska file is also marked as keyframe in the matroska stream. tcpserversink only looks at a) not b). I'm not sure b) is right. > probably yes, however this was buggy even before this patch so probably > should be a separate commit, what do you think about? Aye.
Created attachment 328706 [details] [review] updated patch
Thanks. There's just one possible issue with this now, and that's that the patch increases file size by ca. 2.6% (based on a test with a random aac file). Question is if that's acceptable or if we want to try and reduce that a bit more.
yes I can confirm, using this pipeline: audiotestsrc num-buffers=1000 ! mulawenc ! matroskamux ! filesink without the patch the produced file is 1072513 bytes, with the patch 1093422 bytes, so about 2% bigger. Do you want a property so users can activate this behaviour only if they need to stream via tcpserversink or similar use cases?
Perhaps we can just do something smarter, like not start a new cluster for every single frame or so?
a new cluster each x frames with x configurable via a property or hardcoded is good enough? This however will introduce delay when streaming, in my use case I'll set to 1
This is very similar to https://bugzilla.gnome.org/show_bug.cgi?id=766618 btw. Ideally we also do lacing for audio here :)
(In reply to Sebastian Dröge (slomo) from comment #17) > This is very similar to https://bugzilla.gnome.org/show_bug.cgi?id=766618 > btw. > mpegtsmux seems to produce a very big overhead, here we are speaking about 2-3%, anyway do you want a max-delay property as suggested in that bug or an hardcoded value? Probably here we can simply count bytes/buffers and create a new cluster when they exceed a threshold > Ideally we also do lacing for audio here :)
The use case is the following: - stream audio only media inside a matroska container, audio + video or video only is not affected. Saving to file is not affected matroskamux already has a streamable property that users probably set to "true" if they want a network streaming so 1) we could use the approach proposed here for audio only stream only if streamable is true 2) we can add a new property "max-audio-only-stream-delay" or with a better name :), if set to -1 does nothing (actual behaviour and default value), 0 mark as keyframe each audio buffer (only if streamble is true?), xxx wait for xxx time (based on buffer duration) or bytes before mark the audio buffer as keyframe (only if streamble is true?) 3) other options and or combination of the first two? what is the consensus here?
any news about this?
1) seems sensible independently too, Tim do you think this is good for merging? 2), proper lacing for audio, should be implemented nonetheless. With a property instead of magic, just like it should be in mpegtsmux.
commit 38265ee1a50b2288dd38a4a9edf8b5e8b01d624c Author: Nicola Murino <nicola.murino@gmail.com> Date: Mon May 30 01:15:31 2016 +0200 matroskamux: mark all packets of audio-only streams as keyframes This helps with streaming audio-only streams via multifdsink, tcpserversink and such. https://bugzilla.gnome.org/show_bug.cgi?id=754696
This seems to break 'make check': elements/matroskamux.c:189:F:general:test_block_group:0: Assertion 'gst_buffer_memcmp (buffer, 0, data, data_size) == 0' failed (I could swear I tested this before pushing..)
Created attachment 342318 [details] [review] fix make check Now the audio buffers have extra matroska headers
Comment on attachment 342318 [details] [review] fix make check Thanks for the patch, but could you please write a proper commit message with an explanation for the change :) https://gstreamer.freedesktop.org/documentation/contribute/#writing-good-commit-messages
Created attachment 342340 [details] [review] updated patch I hope this is better, I used this https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=4e5c985ba439a1646494a798e8b31988d9bd449f as inspiration
Comment on attachment 342340 [details] [review] updated patch Thanks :) commit 8fe478c8a7746cd2c63f20d23e97e26e1a0e6192 Author: Nicola Murino <nicola.murino@gmail.com> Date: Wed Dec 21 17:43:58 2016 +0100 matroskamux: adjust unit test to modified behaviour Now matroskamux mark all packets of audio-only streams as keyframes so in test_block_group after pushing the test audio data 4 buffers are produced and not more 2. The last buffer is the original data and must match with what pushed. The remaining ones are matroskamux headers https://bugzilla.gnome.org/show_bug.cgi?id=754696
For Opus in Matroska, this causes 10% overhead.
Created attachment 355689 [details] [review] add interleave properties this patch reduce help to reduce the overhead setting the interleave properties, anyway please note that if streamable is false much overhead is caused by index creation: for audio only file an index is added for each buffer. Before of my patch there was no idex at all for audio only streams. Do you want to restore this behaviour or do you want to reduce the number of indexes?
Created attachment 355704 [details] [review] alternative patch in this patch interleave properties influence the indexes creation too
Let's handle this in https://bugzilla.gnome.org/show_bug.cgi?id=784971
Review of attachment 355704 [details] [review]: ::: gst/matroska/matroska-mux.c @@ +82,3 @@ #define DEFAULT_TIMECODESCALE GST_MSECOND +#define DEFAULT_INTERLEAVE_BYTES 0 +#define DEFAULT_INTERLEAVE_TIME 0 Maybe the default should be higher, 500ms and 0 bytes maybe? Also this is not really about interleave, it's about how often to write a "sync point". I think the bytes property is also not that useful because of that, and just the time property would be enough. @@ +359,3 @@ + g_param_spec_uint64 ("interleave-time", "Interleave (time)", + "Interleave between matroska headers/indexes in nanoseconds. A new matroska " + "header will be created irrespetive of this property if max cluster " Typo (also in the other property): irrespeCtive
Ok, so the new property name could be "min-cluster-duration" (but this is also the minimum time distance between indexes), do you want a "max-cluster-duration" too? (using the actual hard coded value as default value?) here is some relevant code from mkvtoolnix about max cluster duration https://github.com/mbunkus/mkvtoolnix/blob/master/src/merge/output_control.cpp#L103 https://github.com/mbunkus/mkvtoolnix/blob/master/src/merge/mkvmerge.cpp#L1688
Makes sense, yes