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.

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.

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).

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.

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.

Review of attachment 338009 [details] [review]:

Looks good.

Makes sense. What do you think about having some default values there for all codecs?

(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?

Yeah, something like 250-500ms seems sensible to me.

(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.

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.

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.

Created attachment 338106 [details] [review]
qtmux: Use a default interleave of 250ms for all codecs

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