[Bug 684790] isomp4/qtdemux: Whenever a moov atom is found, restart the demuxer

[GStreamer (GNOME Bugzilla)]

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

Edward Hervey <bilboed at bilboed.com> changed:

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

--- Comment #32 from Edward Hervey <bilboed at bilboed.com> ---
Review of attachment 349904:
 --> (https://bugzilla.gnome.org/review?bug=684790&attachment=349904)

::: gst/isomp4/qtdemux.c
@@ +97,3 @@
 #define QTDEMUX_TREE_NODE_FOURCC(n) (QT_FOURCC(((guint8 *) (n)->data) + 4))

+#define STREAM_IS_EOS(s) ((s)->time_position == GST_CLOCK_TIME_NONE)

Put as a separate patch.

@@ +102,3 @@

+#define QTDEMUX_NTH_STREAM(demux,i) ((QtDemuxStream
*)g_list_nth_data(demux->active_streams,i))
+#define QTDEMUX_N_STREAMS(demux) (g_list_length(demux->active_streams))

This summarizes my main "issue" with this patch.

Using a linked list instead of an array doesn't bring much overhead for qtdemux
but:
1) We need to avoid using g_list_nth_data() as much as possible
2) We need to avoid using g_list_length() as much as possible

They are both functions that iterate over all/most of the list.

n_streams => Just use/update the n_streams variable, which you update whenever
a stream is added/removed

nth_stream => In virtually all scenarios we are either:
* Using the first entry  (just use it directly)
* Using an index which we calculated in the same function while iterating (just
remember the stream entry)

@@ +2093,2 @@
   if (hard) {
+    for (iter = qtdemux->active_streams; iter; iter = next) {

Use g_list_free_full()

@@ +2557,2 @@
 static void
 gst_qtdemux_remove_stream (GstQTDemux * qtdemux, int i)

This could just be modified to be (GstQTDemux *demux, GstQTDemuxStream
*stream);

And then you can do active_streams = g_list_remove(active_streams, stream);

@@ +2654,3 @@
     return;

+  stream = QTDEMUX_NTH_STREAM (qtdemux, 0);

stream = (GstQTDemuxStream*) qtdemux->active_streams->data;

Or make a macro QTDEMUX_FIRST_STREAM() if you want

@@ +3377,3 @@
   }

   /* try to get it fast and simple */

This "optimization" needs to go away if we use a linked list, because we can no
longer do direct access.

@@ +5732,2 @@
     GST_DEBUG_OBJECT (qtdemux, "we reached the end of our segment.");
+    QTDEMUX_NTH_STREAM (qtdemux, index)->time_position = GST_CLOCK_TIME_NONE;

Remember the stream in the previous iteration instead of using NTH_STREAM

@@ +6125,3 @@
     return -1;

+  stream = QTDEMUX_NTH_STREAM (demux, smallidx);

Store the stream object in the previous iteration.

@@ +11616,2 @@
   /* now we are ready to add the stream */
+  if (QTDEMUX_N_STREAMS (qtdemux) >= GST_QTDEMUX_MAX_STREAMS)

We no longer have this limitation by switching to a linked list :D

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