[Bug 777825] isoff: Move isoff to gst-libs

[GStreamer (GNOME Bugzilla)]

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

Reynaldo H. Verdejo Pinochet <reynaldo at osg.samsung.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #352560|none                        |needs-work
             status|                            |

--- Comment #33 from Reynaldo H. Verdejo Pinochet <reynaldo at osg.samsung.com> ---
Review of attachment 352560:
 --> (https://bugzilla.gnome.org/review?bug=777825&attachment=352560)

Thanks for staying on top of this. Appreciated.

Code can be improved a bit but I see no blockers. Just
a few suggestions:

::: gst-libs/gst/isoff/gstisoff.c
@@ +425,3 @@
+    if (!gst_byte_reader_skip (reader, 8))
+      return FALSE;
+

Looks like the skip is actually the only conditional step and you can take the
_get_uint32_be() outside the if/else if/else. Also, a switch would make the
version logic easier to maintain and seems just better suited for the task.

@@ +482,3 @@
+      }
+      case GST_ISOFF_FOURCC_HDLR:{
+

Nit, feel free to ignore: move sub_reader declaration to the top and save
writing it twice?

@@ +496,3 @@
+  }
+
+    return FALSE;

There's no need for two $had_ variables. Use only one and consider making the
function return the found value instead of a boolean if you think it's useful.

@@ +526,3 @@
+  } else {
+    return FALSE;
+    guint64 size;

If future new $version values are possible please make it a switch. If not the
whole block may be replaced by something simpler like (double check, drafted):

if (version != 0 && version != 1)
  return FALSE;
if (!gst_byte_reader_skip (reader, version? 8:16))
  return FALSE;

@@ +562,3 @@
+      }
+      case GST_ISOFF_FOURCC_TKHD:{
+      case GST_ISOFF_FOURCC_HDLR:{

Nit, feel free to ignore: move sub_reader declaration to the top and save
writing it twice?

@@ +578,3 @@
+  }
+
+      }

Again, there's no need for two $had_ with the logic in place. Make it one and
consider returning an mdia or tkhd flag if worth anything.

@@ +619,3 @@
+      default:
+        gst_byte_reader_skip (reader, size - header_size);
+    return FALSE;

At a first glance it seems like you can drop all the had_track variable logic
if you simple "goto error;" before breaking.

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