[Bug 755036] mssdemux: improved live playback support

[GStreamer (GNOME Bugzilla)]

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

--- Comment #14 from Philippe Normand <philn at igalia.com> ---
(In reply to Matthew Waters (ystreet00) from comment #13)
> Review of attachment 339338 [details] [review]:
> 
> The fragment parsing feels like it should go into qtdemux instead.  Any
> reason why it isn't?
> 

I agree it is a bit weird indeed. It could go to qtdemux but then I think we
would need relay the parsing results back to the mss demuxer somehow.

> ::: ext/smoothstreaming/gstmssmanifest.c
> @@ +78,3 @@
>  
> +  gboolean has_live_fragments;
> +  GQueue live_fragments;
> 
> Why the separate variable instead of changing over the existing GList
> fragments?
> 

It can be changed to use the existing list. I first thought it was better to
differenciate the fragments coming from the manifest from the ones built during
the current fragment parsing.

> @@ +267,3 @@
> +      manifest->is_live ? "yes" : "no",
> manifest->look_ahead_fragment_count);
> +  stream->has_live_fragments = manifest->is_live
> +      && manifest->look_ahead_fragment_count;
> 
> What exactly is the has_live_fragments condition signalling?
> 
> The possible existence of parsing needed for the TfrfBox?
> 

Yes

> @@ +1117,3 @@
> +    fragment = stream->current_fragment->data;
> +    if (fragment->time <= prev_fragment->time) {
> +      while (fragment->time <= prev_fragment->time) {
> 
> double condition without an else on the if seems a little pointless no?
> 
> Also, what exactly is the while loop meant to solve?
> 
> When aren't the fragments that you're iterating over, not in order?
> 
> In any case, this seems like it should be a separate patch.

Yes this could be split out I think. Currently I don't remember exactly why I
wrote this :)
I'll check it again.

> 
> @@ +1526,3 @@
> +
> +void
> +gst_mss_stream_fragment_parse (GstMssStream * stream, GstBuffer * buffer)
> 
> parse_fragment() is a better name.
> 

Surely can be renamed yes :)

> @@ +1545,3 @@
> +
> +  for (guint8 index = 0; index < stream->fragment_parser.tfrf.entries_count;
> +      index++) {
> 
> Any particular reason you chose an 8-bit integer for an index?
> 
> That would only allow 255 fragments.

The streams I've tested never had more than 2 look-ahead fragments. But yeah,
that 8-bit integer can be changed, though I suppose it's unlikely to have more
than 255 look-ahead fragments.

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