[Bug 769048] qtmux: prores-related fixes

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=769048

Thiago Sousa Santos <thiagossantos at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #334015|none                        |reviewed
             status|                            |

--- Comment #9 from Thiago Sousa Santos <thiagossantos at gmail.com> ---
Review of attachment 334015:
 --> (https://bugzilla.gnome.org/review?bug=769048&attachment=334015)

Thanks for the patches, just some minor comments.

::: gst/isomp4/atoms.c
@@ +4026,3 @@

+  par_n = entry->par_n;
+  par_d = entry->par_d;

Is this guaranteed to be non-zero or are we risking setting 0:0 as par?

::: gst/isomp4/gstqtmux.c
@@ +4209,3 @@
+        !g_strcmp0 (interlace_mode, "mixed")) {
+      /* Assume top-fields-first if unspecified */
+      gst_structure_get_boolean (structure, "top-field-first", &tff);

are there more types than top-field-first and bottom-field-first? Maybe it
would be better to have a string? Does interleave/deinterleave have any caps
for this already?

@@ +4220,3 @@
+          "Colorimetry information not found in caps. The resulting file's "
+          "color information might be wrong");
+    ext_atom = build_fiel_extension_prores (interlace_mode, tff);

Is this mandatory? Do we have to guess? Anyway this is only used for prores so
it shouldn't matter for other formats.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.