[Bug 744362] dashdemux: Fix handling of live streams with timeshift buffers.

[bugzilla at gnome.org]

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

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

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

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

Nice addition!

just 2 observations below.

::: ext/dash/gstmpdparser.c
@@ +3859,3 @@
+    g_date_time_unref (now);
+    g_date_time_unref (start);
+    duration = stream_now * GST_USECOND;

This has the side effect of making the duration query return a duration for a
live stream. I don't know an use case where knowing the 'current duration' of a
live stream would be useful so I'd avoid that. Some applications can query the
duration and then store that as a permanent value, this can lead to issues
later. Even if you wanted to do that, you'd need to remember to post a
duration-changed message everytime a new fragment was generated.

So, while the code above is correct I'd keep it separate from the usual
'get_duration' call to prevent problems later.

::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c
@@ +653,2 @@
   if (first_segment)
+    demux->segment.start = demux->segment.position = demux->segment.time =
min_pts;

Why set the segment.time here?

It doesn't seem related to the fix and it would deserve some testing with HLS
streams to see what would be shown with this set?

Anyway I think this should be in a different patch as it doesn't seem to be
part of the feature you are adding.

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