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 754696 - matroskamux: audio-only streams have all buffers flagged as delta units, causing problems with tcpserversink/multifdsink
matroskamux: audio-only streams have all buffers flagged as delta units, caus...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-07 16:25 UTC by Nicola
Modified: 2017-07-18 06:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (2.33 KB, patch)
2015-09-08 09:53 UTC, Nicola
none Details | Review
updated patch (2.10 KB, patch)
2016-05-29 23:16 UTC, Nicola
committed Details | Review
fix make check (1.42 KB, patch)
2016-12-21 14:10 UTC, Nicola
none Details | Review
updated patch (2.16 KB, patch)
2016-12-21 16:46 UTC, Nicola
committed Details | Review
add interleave properties (5.71 KB, patch)
2017-07-15 15:07 UTC, Nicola
none Details | Review
alternative patch (6.18 KB, patch)
2017-07-16 11:23 UTC, Nicola
needs-work Details | Review

Description Nicola 2015-09-07 16:25:39 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?
Comment 1 Tim-Philipp Müller 2015-09-07 16:42:44 UTC
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.
Comment 2 Nicola 2015-09-07 17:05:07 UTC
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
Comment 3 Tim-Philipp Müller 2015-09-07 17:12:17 UTC
You suggest the muxer should do this unconditionally? or only if there's no video stream?
Comment 4 Nicola 2015-09-07 17:22:55 UTC
(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)
Comment 5 Nicola 2015-09-08 09:53:35 UTC
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
Comment 6 Nicola 2015-09-08 10:03:12 UTC
the patch assume that for audio only stream each buffer is managed as a video keyframe
Comment 7 Nicola 2015-09-10 21:21:12 UTC
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
Comment 8 Nicola 2016-02-15 12:26:06 UTC
ping
Comment 9 Tim-Philipp Müller 2016-05-27 19:11:10 UTC
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);

?
Comment 10 Nicola 2016-05-29 22:15:09 UTC
(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?

> 
> ?
Comment 11 Tim-Philipp Müller 2016-05-29 22:43:41 UTC
> > 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.
Comment 12 Nicola 2016-05-29 23:16:30 UTC
Created attachment 328706 [details] [review]
updated patch
Comment 13 Tim-Philipp Müller 2016-06-17 20:26:48 UTC
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.
Comment 14 Nicola 2016-06-17 20:43:05 UTC
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?
Comment 15 Tim-Philipp Müller 2016-06-17 20:59:13 UTC
Perhaps we can just do something smarter, like not start a new cluster for every single frame or so?
Comment 16 Nicola 2016-06-17 21:06:17 UTC
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
Comment 17 Sebastian Dröge (slomo) 2016-06-17 21:10:25 UTC
This is very similar to https://bugzilla.gnome.org/show_bug.cgi?id=766618 btw.

Ideally we also do lacing for audio here :)
Comment 18 Nicola 2016-06-17 22:24:05 UTC
(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 :)
Comment 19 Nicola 2016-06-24 13:54:27 UTC
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?
Comment 20 Nicola 2016-12-19 16:27:27 UTC
any news about this?
Comment 21 Sebastian Dröge (slomo) 2016-12-20 10:53:08 UTC
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.
Comment 22 Tim-Philipp Müller 2016-12-20 23:49:48 UTC
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
Comment 23 Tim-Philipp Müller 2016-12-21 00:46:42 UTC
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..)
Comment 24 Nicola 2016-12-21 14:10:12 UTC
Created attachment 342318 [details] [review]
fix make check

Now the audio buffers have extra matroska headers
Comment 25 Tim-Philipp Müller 2016-12-21 14:47:55 UTC
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
Comment 26 Nicola 2016-12-21 16:46:45 UTC
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 27 Tim-Philipp Müller 2016-12-21 17:00:11 UTC
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
Comment 28 Sebastian Dröge (slomo) 2017-07-15 07:10:28 UTC
For Opus in Matroska, this causes 10% overhead.
Comment 29 Nicola 2017-07-15 15:07:25 UTC
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?
Comment 30 Nicola 2017-07-16 11:23:55 UTC
Created attachment 355704 [details] [review]
alternative patch

in this patch interleave properties influence the indexes creation too
Comment 31 Sebastian Dröge (slomo) 2017-07-17 08:07:00 UTC
Let's handle this in https://bugzilla.gnome.org/show_bug.cgi?id=784971
Comment 32 Sebastian Dröge (slomo) 2017-07-17 12:47:52 UTC
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
Comment 33 Nicola 2017-07-17 13:17:56 UTC
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
Comment 34 Sebastian Dröge (slomo) 2017-07-17 13:30:30 UTC
Makes sense, yes