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 784971 - matroskamux: Writes one cluster per audio frame
matroskamux: Writes one cluster per audio frame
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.12.0
Other Linux
: Normal blocker
: 1.12.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-07-15 07:05 UTC by Sebastian Dröge (slomo)
Modified: 2017-07-18 07:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
updated patch (6.31 KB, patch)
2017-07-17 22:35 UTC, Nicola
committed Details | Review

Description Sebastian Dröge (slomo) 2017-07-15 07:05:24 UTC
> gst-launch-1.0 audiotestsrc num-buffers=1000 ! opusenc ! matroskamux ! filesink location=test.mka

If you look with mkvinfo, each Opus frame gets its own cluster. That's introducing a huge amount of overhead. At least having multiple frames per block would be good, even better if we could do lacing.
Comment 1 Sebastian Dröge (slomo) 2017-07-15 07:08:35 UTC
This seems to be intentional, but not very useful IMHO

>    if (mux->cluster_time +
>        mux->max_cluster_duration < buffer_timestamp
>        || is_video_keyframe || mux->force_key_unit_event || is_audio_only) {

Maybe we should make max_cluster_duration configurable (it currently is G_MAXINT16 * mux->time_scale).


This was introduced with

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 2 Sebastian Dröge (slomo) 2017-07-15 07:10:40 UTC
For some numbers, this causes 10% overhead for Opus.
Comment 3 Sebastian Dröge (slomo) 2017-07-17 08:07:18 UTC
Patches in https://bugzilla.gnome.org/show_bug.cgi?id=754696#c29
Comment 4 Nicola 2017-07-17 08:12:58 UTC
actually max_cluster_duration is indirectly configurable since time_scale can be setted using a property, please let me know what changes I need to do to the patch submitted in the other bug
Comment 5 Nicola 2017-07-17 22:35:15 UTC
Created attachment 355788 [details] [review]
updated patch

I removed the part about the index creation since there is already a property for that (min-index-interval).

please note the condition "buffer_timestamp > mux->cluster_time" in duration calculation: this is a problem if there is a big gap backward

You need also to revert this

https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=8fe478c8a7746cd2c63f20d23e97e26e1a0e6192
Comment 6 Sebastian Dröge (slomo) 2017-07-18 07:04:21 UTC
Review of attachment 355788 [details] [review]:

Merged with minor changes, thanks!

::: gst/matroska/matroska-mux.c
@@ +352,3 @@
+      g_param_spec_int64 ("min-cluster-duration", "Minimum cluster duration",
+          "Desidered cluster duration as nanoseconds. A new cluster will be "
+          "created irrispective of this property if a force key unit event "

irrEspective

@@ +3630,3 @@
+  is_max_duration_exceeded = (mux->max_cluster_duration > 0
+      && buffer_timestamp > mux->cluster_time
+      && (buffer_timestamp - mux->cluster_time) > mux->max_cluster_duration);

I made both checks >= mux->mXX_cluster_duration. Otherwise if e.g. exactly 500ms are possible, you always create 500+Xms clusters :)
Comment 7 Sebastian Dröge (slomo) 2017-07-18 07:08:54 UTC
commit 7e718d6039cc652a0ced10862768517d5a0b9829 (HEAD -> master)
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Jul 18 10:01:13 2017 +0300

    Revert "matroskamux: adjust unit test to modified behaviour"
    
    This reverts commit 8fe478c8a7746cd2c63f20d23e97e26e1a0e6192.
    
    We're back to previous behaviour

commit 1bbdfa8738da6ecfe73cbead4af74e3d4172b389
Author: Nicola Murino <nicola.murino@gmail.com>
Date:   Tue Jul 18 00:26:11 2017 +0200

    matroskamux: add properties to control cluster duration
    
    https://bugzilla.gnome.org/show_bug.cgi?id=784971
Comment 8 Sebastian Dröge (slomo) 2017-07-18 07:13:19 UTC
Will backport to 1.12 later. Now it would be nice to also write SimpleBlocks for Opus, but that's a separate issue. For that we would have to know the default block duration
Comment 9 Sebastian Dröge (slomo) 2017-07-18 07:44:46 UTC
(In reply to Sebastian Dröge (slomo) from comment #8)
> Now it would be nice to also write SimpleBlocks
> for Opus, but that's a separate issue. For that we would have to know the
> default block duration

See https://bugzilla.gnome.org/show_bug.cgi?id=784969#c5