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 781447 - qtmux: Add new prefill recording mode
qtmux: Add new prefill recording mode
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other All
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-04-18 11:36 UTC by Sebastian Dröge (slomo)
Modified: 2017-05-09 12:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtmux: Add new prefill recording mode (43.97 KB, patch)
2017-04-18 11:36 UTC, Sebastian Dröge (slomo)
none Details | Review
qtmux: Add new prefill recording mode (44.26 KB, patch)
2017-04-18 17:29 UTC, Sebastian Dröge (slomo)
none Details | Review
qtmux: Add new prefill recording mode (44.26 KB, patch)
2017-05-09 11:07 UTC, Sebastian Dröge (slomo)
none Details | Review
qtmux: Add new prefill recording mode (44.81 KB, patch)
2017-05-09 12:13 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Sebastian Dröge (slomo) 2017-04-18 11:36:03 UTC
See commit message. This is what software like Adobe Premiere and FinalCut Pro
expect to see for files that are still written to.
Comment 1 Sebastian Dröge (slomo) 2017-04-18 11:36:10 UTC
Created attachment 349993 [details] [review]
qtmux: Add new prefill recording mode

This sets up a moov with the correct sample positions beforehand and
only works with constant framerate, I-frame only streams.

Currently only support for ProRes and raw audio is implemented but
adding new codecs is just a matter of defining appropriate maximum frame
sizes.
Comment 2 Jan Schmidt 2017-04-18 13:10:19 UTC
Review of attachment 349993 [details] [review]:

Mostly looks good. Some small comments.

::: gst/isomp4/gstqtmux.c
@@ +296,3 @@
 #define DEFAULT_RESERVED_MOOV_UPDATE_PERIOD   GST_CLOCK_TIME_NONE
 #define DEFAULT_RESERVED_BYTES_PER_SEC_PER_TRAK 550
+#define DEFAULT_RESERVED_PREFILL TRUE

Should this be TRUE?

@@ +2214,3 @@
+    default:
+      GST_ERROR_OBJECT (qtmux, "unsupported codec for pre-filling");
+      return -1;

Slightly strange to put the default case handler in the middle of the switch.

@@ +2375,3 @@
+      if (walk == NULL) {
+        qpad->expected_sample_duration_n = 25;
+        qpad->expected_sample_duration_d = 1;

Might want some kind of discoverable log output for this fallback case?

@@ +2439,3 @@
+static gboolean
+gst_qt_mux_prefill_samples (GstQTMux * qtmux)
+{

The result of this function isn't checked anywhere, and doesn't seem to generate any GST_ELEMENT_ERROR on unsupported codecs
Comment 3 Sebastian Dröge (slomo) 2017-04-18 17:29:23 UTC
Created attachment 350012 [details] [review]
qtmux: Add new prefill recording mode

This sets up a moov with the correct sample positions beforehand and
only works with constant framerate, I-frame only streams.

Currently only support for ProRes and raw audio is implemented but
adding new codecs is just a matter of defining appropriate maximum frame
sizes.
Comment 4 Sebastian Dröge (slomo) 2017-04-18 17:34:31 UTC
Thanks!
Comment 5 Sebastian Dröge (slomo) 2017-05-09 11:07:57 UTC
Created attachment 351423 [details] [review]
qtmux: Add new prefill recording mode

This sets up a moov with the correct sample positions beforehand and
only works with constant framerate, I-frame only streams.

Currently only support for ProRes and raw audio is implemented but
adding new codecs is just a matter of defining appropriate maximum frame
sizes.
Comment 6 Sebastian Dröge (slomo) 2017-05-09 12:13:38 UTC
Created attachment 351427 [details] [review]
qtmux: Add new prefill recording mode

This sets up a moov with the correct sample positions beforehand and
only works with constant framerate, I-frame only streams.

Currently only support for ProRes and raw audio is implemented but
adding new codecs is just a matter of defining appropriate maximum frame
sizes.
Comment 7 Sebastian Dröge (slomo) 2017-05-09 12:19:59 UTC
Attachment 351427 [details] pushed as 55f9496 - qtmux: Add new prefill recording mode