After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 757619 - hlsdemux: incorrect segment start value on bitrate switch
hlsdemux: incorrect segment start value on bitrate switch
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.6.1
Other All
: Normal normal
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-11-05 05:58 UTC by Rajat Verma
Modified: 2015-11-16 04:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
adaptivedemux: Create a new segment event when switching pads because of bitrate changes (1.26 KB, patch)
2015-11-06 10:39 UTC, Sebastian Dröge (slomo)
rejected Details | Review

Description Rajat Verma 2015-11-05 05:58:35 UTC
On playing bipbop stream (https://devimages.apple.com.edgekey.net/streaming/examples/bipbop_4x3_variant/bipbop_4x3_variant.m3u8), incorrect segment start values are sent on bitrate switch

In this case, bitrate is switched at 10 sec, so segment event that was sent on hlsdemux0:src1 should have its start value as 10 sec instead of 0.

Please see below logs:

0:00:01.374058602 23797 0x7f8a3c002630 DEBUG          adaptivedemux gstadaptivedemux.c:1771:gst_adaptive_demux_stream_push_buffer:<hlsdemux0:src_0> Sending pending seg: segment event: 0x7f8a3c003e70, time 99:99:99.999999999, seq-num 54, GstEventSegment, segment=(GstSegment)"GstSegment, flags=(GstSegmentFlags)GST_SEGMENT_FLAG_NONE, rate=(double)1, applied-rate=(double)1, format=(GstFormat)GST_FORMAT_TIME, base=(guint64)0, offset=(guint64)0, start=(guint64)0, stop=(guint64)18446744073709551615, time=(guint64)0, position=(guint64)0, duration=(guint64)18446744073709551615;";
0:00:06.279231534 23797 0x7f8a40119a80 DEBUG          adaptivedemux gstadaptivedemux.c:1771:gst_adaptive_demux_stream_push_buffer:<hlsdemux0:src_1> Sending pending seg: segment event: 0x7f89f8000c00, time 99:99:99.999999999, seq-num 54, GstEventSegment, segment=(GstSegment)"GstSegment, flags=(GstSegmentFlags)GST_SEGMENT_FLAG_NONE, rate=(double)1, applied-rate=(double)1, format=(GstFormat)GST_FORMAT_TIME, base=(guint64)0, offset=(guint64)0, start=(guint64)0, stop=(guint64)18446744073709551615, time=(guint64)0, position=(guint64)0, duration=(guint64)18446744073709551615;";
Comment 1 Sebastian Dröge (slomo) 2015-11-05 08:13:12 UTC
All the segment calculations are inside the adaptivedemux base class. Can you check how it gets to these values, and what informations it gets from hlsdemux for the calculations?

A segment with start 0 would only make sense here if it didn't switch pads, it switched pads so it should make the running time of 10s be 0s, and the stream time be 10s.
Comment 2 Rajat Verma 2015-11-05 09:17:36 UTC
Currently in adaptive demux,  gst_adaptive_demux_stream_update_fragment_info () is only called for first_segment (or seek).

That's why, segment.start always comes to 0 on switching bitrate.

If I move this out of first_segment check, segment.start values come correct on switching bitrates.

However, I see below commit, which moved this inside first_segment check specifically.

commit 0bef1974e2dcdfff1b243a048d940540938f51c5
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Tue Dec 23 01:48:41 2014 -0300

    adaptivedemux: fix segment start when exposing new streams
    
    Segment start needs only to be updated when starting the streams
    or after a seek, doing it during bitrate changes will cause the
    running time to go discontinuous (jump back to a previous ts)
    and QOS will drop buffers
Comment 3 Sebastian Dröge (slomo) 2015-11-05 10:02:34 UTC
Thiago do you remember why you did this change? I think on group switches (i.e. switching pads) we will have to update the segment in a way that the first buffer coming out there has running time 0 (and stream time according to the manifest). It basically should be the same segment you would create for a flushing seek to that position.
Comment 4 Thiago Sousa Santos 2015-11-05 15:07:11 UTC
(In reply to Sebastian Dröge (slomo) from comment #3)
> Thiago do you remember why you did this change? I think on group switches
> (i.e. switching pads) we will have to update the segment in a way that the
> first buffer coming out there has running time 0 (and stream time according
> to the manifest). It basically should be the same segment you would create
> for a flushing seek to that position.

Why would we have to go back to running time 0? Shouldn't the running time end up being the same as the last pushed buffer in the previous played segment? Otherwise the sink will consider late all the buffers up to that time.

I believe the rationale for that patch is that all streams in the manifest should have an aligned start time, so they all should run under the same segment. It could also be achieved by adjusting start, base and time in the segment but having the same segment makes it easier for debugging.

What is the value of the timestamp of the first pushed buffer after the switch?
Comment 5 Sebastian Dröge (slomo) 2015-11-05 22:17:40 UTC
Currently group switches in playbin are handled by streamsynchronizer, which assumes that running time starts from 0 again and fixes it up to be continuous with what was there before. It basically works the same as gapless playback in playbin.

That's why things would now fail otherwise.


I agree that this is weird and seems wrong but there's not much we can do about that in the current decodebin.



The timestamp of the first pushed buffer after a switch should be the stream time of the switch, but I'll double check tomorrow.
Comment 6 Rajat Verma 2015-11-06 05:17:19 UTC
>>The timestamp of the first pushed buffer after a switch should be the stream time of the switch

Yes, it is indeed the stream time of the switch.
Comment 7 Sebastian Dröge (slomo) 2015-11-06 10:39:30 UTC
Created attachment 314971 [details] [review]
adaptivedemux: Create a new segment event when switching pads because of bitrate changes

instead of taking the same segment as before. Switching pads is always as if
we seeked to the new position.
Comment 8 Sebastian Dröge (slomo) 2015-11-06 10:52:01 UTC
This patch seems to make things better sometimes, and worse at other times :)
Comment 9 Jan Schmidt 2015-11-06 10:57:49 UTC
Does streamsynchroniser care if the running time doesn't go back to 0? Doesn't it just make the new running-time gapless against the old segment regardless?
Comment 10 Sebastian Dröge (slomo) 2015-11-06 11:08:13 UTC
I think it assumes 0, but that can be fixed of course
Comment 11 Sebastian Dröge (slomo) 2015-11-06 12:45:29 UTC
OTOH streamsynchronizer does not detect group switches caused by bitrate changes as a stream change... so it does not do anything at all. And in theory passing through the same segment should work fine then

For the above stream, it gives 404 now.
Comment 12 Rajat Verma 2015-11-06 13:14:48 UTC
Looks like this stream is no longer available on server now :(
Comment 13 Sebastian Dröge (slomo) 2015-11-06 18:48:30 UTC
Segments and timestamps look good to me btw, both at adaptivedemux and streamsynchronizer. Not sure what the problem is yet
Comment 15 Sebastian Dröge (slomo) 2015-11-09 10:40:10 UTC
This seems to work well with git master now here, on my test stream and also on yours. The bitrate switch at 10s in your stream is not very smooth though, because the new fragment does not contain a keyframe at the beginning (thanks Apple) but only at 12s, so after the bitrate switch the picture freezes for 2s.

Can you confirm?
Comment 16 Rajat Verma 2015-11-09 10:49:25 UTC
Yes, additionally I can verify this from ffprobe also that it contains keyframe at 12 sec.
Comment 17 Rajat Verma 2015-11-10 03:31:54 UTC
Maybe I missed something here, no patch has been committed and ticket is marked "Resolved/Fixed" with target milestone 1.7.1 ?
Comment 18 Sebastian Dröge (slomo) 2015-11-10 08:27:59 UTC
Well, you also reported that it seemed to work with GIT master apart from the keyframe problem :) Some change apparently fixed it recently, I'm not sure which one though.

Or do you still have another problem with this stream during bitrate switches?
Comment 19 Rajat Verma 2015-11-10 08:46:39 UTC
Oh, sorry for the confusion. I only meant to confirm that this has keyframe at 12 sec.

Also, problem I raised this bug for is incorrect segment start value, that remains unless below patch is merged

>>adaptivedemux: Create a new segment event when switching pads because of bitrate changes
Comment 20 Sebastian Dröge (slomo) 2015-11-10 08:50:19 UTC
Why do you think the segment event is incorrect? My patch breaks things, the segment events are supposed to be like they're not, otherwise things will break further downstream of hlsdemux.

streamsynchronizer in playbin/playsink is expecting such segment events. For bitrate switches there should not be an updated one.


Or do you mean the segment.start should only be adjusted so that older buffers are clipped and not forwarded further downstream? In case there are some. In that case my patch is also wrong as it additionally changes the running time, which it shouldn't.