[gstreamer-bugs] [Bug 601576] qtmux enhancements (moov recovery, mem usage and edit lists)

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=601576
  GStreamer | gst-plugins-bad | 0.10.14

Stefan Kost (gstreamer, gtkdoc dev) <ensonic> changed:

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

--- Comment #11 from Stefan Kost (gstreamer, gtkdoc dev) <ensonic at sonicpulse.de> 2010-01-26 12:10:05 UTC ---
Review of attachment 149998:
 --> (https://bugzilla.gnome.org/review?bug=601576&attachment=149998)

Good work. I only have some minor nitpicks.

::: gst/qtmux/atomsrecovery.c
@@ +84,3 @@
+ * IMPORTANT: this is still at a experimental state.
+ */
+

What about including a version here. The version can be bumped and the tool can
refuse to recover to load mrf files of wrong version.

@@ +211,3 @@
+    GST_WRITE_UINT64_BE (data + 26, 0);
+  }
+

minor nitpick, the two if(sync) could be joined.

@@ +247,3 @@
+}
+#endif
+

remove?

@@ +829,3 @@
+  buffer = g_malloc0 (size);
+  offset = 0;
+

Does this needs to be cleared? Also what is the 1024*1024 - just a decent upper
limmit?

@@ +881,3 @@
+        "Failed to read the ftyp atom from file");
+    return FALSE;
+  }

please use data=malloc(moovrf->prefix_size), instead data[1024*1204] here.

@@ +924,3 @@
+    ATOMS_RECOV_OUTPUT_WRITE_ERROR (err);
+    return FALSE;
+  }

If data[] is only used here and once (where size is staticaly known) below it
can be made small (16 bytes)

@@ +936,3 @@
+    ATOMS_RECOV_OUTPUT_WRITE_ERROR (err);
+    return FALSE;
+  }

again use malloc() for data

@@ +993,3 @@
+      ATOMS_RECOV_OUTPUT_WRITE_ERROR (err);
+      goto fail;
+    }

again malloc() + free() the buffer.

::: gst/qtmux/atomsrecovery.h
@@ +72,3 @@
+  gboolean  sync;
+  gboolean  do_pts;
+  guint64   pts_offset;

minor nitpick: putting the pts_offset before the two gboolean can save some
memory from alignment gaps (can be checked with pahole tool)

::: gst/qtmux/gstqtmux.c
@@ +2491,3 @@
       break;
     case PROP_FAST_START_TEMP_FILE:
+      g_free (qtmux->fast_start_file_path);

unrelated change that could be applied right away separately

::: gst/qtmux/qtmoovrecover.c
@@ +61,3 @@
+
+  if (argc != 4 && argc != 5) {
+    g_print ("Usage: %s moovrecoveryfile mdatfile outputfile [-fs]\n",
argv[0]);

please make this --fast-start, options args should be -f (single letter) or
--long-name (double dash -> long name)

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.