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 773217 - qtmux: Allow configuring the maximum interleave size in bytes/time
qtmux: Allow configuring the maximum interleave size in bytes/time
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other All
: Normal enhancement
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 769048
 
 
Reported: 2016-10-19 11:34 UTC by Sebastian Dröge (slomo)
Modified: 2016-11-01 18:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtmux: Allow configuring the maximum interleave size in bytes/time (16.28 KB, patch)
2016-10-19 11:34 UTC, Sebastian Dröge (slomo)
reviewed Details | Review
qtmux: Use a default interleave when ProRes is used (1.19 KB, patch)
2016-10-19 11:34 UTC, Sebastian Dröge (slomo)
none Details | Review
qtmux: Allow configuring the interleave size in bytes/time (17.09 KB, patch)
2016-10-20 14:46 UTC, Sebastian Dröge (slomo)
committed Details | Review
qtmux: Use a default interleave when ProRes is used (1.16 KB, patch)
2016-10-20 14:46 UTC, Sebastian Dröge (slomo)
committed Details | Review
qtmux: Use a default interleave of 250ms for all codecs (860 bytes, patch)
2016-10-20 14:46 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Sebastian Dröge (slomo) 2016-10-19 11:34:39 UTC
See commit messages for reasoning.
Comment 1 Sebastian Dröge (slomo) 2016-10-19 11:34:45 UTC
Created attachment 338008 [details] [review]
qtmux: Allow configuring the maximum interleave size in bytes/time

Previously we were switching from one chunk to another on every single
buffer. This wastes some space in the headers and, depending on the
software, might depend in more reads (e.g. if the software is reading
multiple samples in one go if they're in the same chunk).

The ProRes guidelines suggest an interleave of 0.5s is common, but
specifies that for ProRes at most 2MB (for SD) and 4MB (for HD) should
be used per chunk. This will be handled in a follow-up commit.
Comment 2 Sebastian Dröge (slomo) 2016-10-19 11:34:50 UTC
Created attachment 338009 [details] [review]
qtmux: Use a default interleave when ProRes is used

The ProRes guidelines suggest an interleave of 0.5s is common, but
specifies that for ProRes at most 2MB (for SD) and 4MB (for HD) should
be used per chunk.

It might also make sense to use similar numbers in general.
Comment 3 Sebastian Dröge (slomo) 2016-10-19 11:37:19 UTC
Should we use such settings as a default in general? Only for MOV? Thoughts? :)


Also the careful reviewer will notice that the patch also changes qtmux to write the last sample in a more useful place if one stream is shorter than the others. E.g. if there was a duration difference of 1m, before the patch the last sample of the shorter track would be stored right next to the last sample of the longer track (that is: 1m away from the previous sample of the shorter track).
Comment 4 Thiago Sousa Santos 2016-10-20 13:23:47 UTC
Review of attachment 338008 [details] [review]:

Looks good, but perhaps just naming the property interleave-time and interleave-bytes without the 'max' makes it clearer?

I feel that the max passes the impression that it will interleave as usual by time but will start a new chunk once the threshold is reached while this property is actually a fixed interleaving interval. It will consume from pads until it is reached. In short, 'max' passes the impression of threshold but anything below it is fine while without the 'max' gives me a 'target' impression.

::: gst/isomp4/gstqtmux.c
@@ +3569,3 @@
+      if (tmp_buf)
+        gst_buffer_unref (tmp_buf);
+    }

I'd move this early pad selection to a separate function, but this can be done as a separate commit just to make things more modular.
Comment 5 Thiago Sousa Santos 2016-10-20 13:23:54 UTC
Review of attachment 338008 [details] [review]:

Looks good, but perhaps just naming the property interleave-time and interleave-bytes without the 'max' makes it clearer?

I feel that the max passes the impression that it will interleave as usual by time but will start a new chunk once the threshold is reached while this property is actually a fixed interleaving interval. It will consume from pads until it is reached. In short, 'max' passes the impression of threshold but anything below it is fine while without the 'max' gives me a 'target' impression.

::: gst/isomp4/gstqtmux.c
@@ +3569,3 @@
+      if (tmp_buf)
+        gst_buffer_unref (tmp_buf);
+    }

I'd move this early pad selection to a separate function, but this can be done as a separate commit just to make things more modular.
Comment 6 Thiago Sousa Santos 2016-10-20 13:25:25 UTC
Review of attachment 338009 [details] [review]:

Looks good.
Comment 7 Sebastian Dröge (slomo) 2016-10-20 13:36:20 UTC
Makes sense. What do you think about having some default values there for all codecs?
Comment 8 Thiago Sousa Santos 2016-10-20 13:51:04 UTC
(In reply to Sebastian Dröge (slomo) from comment #7)
> Makes sense. What do you think about having some default values there for
> all codecs?

I guess we can do something time-based by default so we don't have to do resolution -> bytes conversions?
Comment 9 Sebastian Dröge (slomo) 2016-10-20 14:07:25 UTC
Yeah, something like 250-500ms seems sensible to me.
Comment 10 Thiago Sousa Santos 2016-10-20 14:10:26 UTC
(In reply to Sebastian Dröge (slomo) from comment #9)
> Yeah, something like 250-500ms seems sensible to me.

Looks good to me. I'd go with 250ms to have a smaller impact to the 0s we use currently but feel free to ignore me if you think 500ms is still safe enough for buffers.
Comment 11 Sebastian Dröge (slomo) 2016-10-20 14:46:38 UTC
Created attachment 338104 [details] [review]
qtmux: Allow configuring the interleave size in bytes/time

Previously we were switching from one chunk to another on every single
buffer. This wastes some space in the headers and, depending on the
software, might depend in more reads (e.g. if the software is reading
multiple samples in one go if they're in the same chunk).

The ProRes guidelines suggest an interleave of 0.5s is common, but
specifies that for ProRes at most 2MB (for SD) and 4MB (for HD) should
be used per chunk. This will be handled in a follow-up commit.
Comment 12 Sebastian Dröge (slomo) 2016-10-20 14:46:43 UTC
Created attachment 338105 [details] [review]
qtmux: Use a default interleave when ProRes is used

The ProRes guidelines suggest an interleave of 0.5s is common, but
specifies that for ProRes at most 2MB (for SD) and 4MB (for HD) should
be used per chunk.

It might also make sense to use similar numbers in general.
Comment 13 Sebastian Dröge (slomo) 2016-10-20 14:46:48 UTC
Created attachment 338106 [details] [review]
qtmux: Use a default interleave of 250ms for all codecs
Comment 14 Sebastian Dröge (slomo) 2016-11-01 18:43:13 UTC
Attachment 338104 [details] pushed as c222578 - qtmux: Allow configuring the interleave size in bytes/time
Attachment 338105 [details] pushed as 4eaf5ea - qtmux: Use a default interleave when ProRes is used
Attachment 338106 [details] pushed as c709abb - qtmux: Use a default interleave of 250ms for all codecs