-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HLS Support for Live Events #848
Conversation
Hi. I'll get to this soon (sorry!). |
Would it be possible to add a sample to the HLS demo project that shows off this code? It is not clear if this PR fixes HLS live seeking. Does it? |
Note that we have issues elsewhere saying "make the demo app simpler". So it's pretty unclear whether that's a good idea :)... |
I added two commits to enable event support in the demo application. I think c64a1cf is okay, but I'm not so sure about 36f7f7e. Passing the listener information (the seek range info) into For testing purposes, I only have a dummy stream at http://demo.castlabs.com/media/BBB-HLS/BBB-Event.m3u8 . It's an |
@thasso thanks for the testing information. I tried your BBB stream and an iTV24 live stream to test your code (from your repo) but get no joy. The player controls start and end time are both 0. Is there more that I need to do to test this PR? Here's what I added to Samples.java: |
@pajatopmr I have a hard time reproducing this for BBB. Here, the playback for BBB starts at 7:40 and the duration is displayed as 8:10 correctly (this is not a real event, so the duration will not change). There are not further changes required to make this work. Do you see output like For the iTV24 stream, this is expected behaviour for this PR. The stream playlists are not marked as "Event" so this PR has no effect and the player as well as the controls behave as before. |
@thasso I do see the available range in the logcat after correctly merging your change. Should not a fixed size live window also report an available seek range change every |
@pajatopmr Can you please double check that you are using the correct branch to test this, or maybe add some logging statements in the You might be right about the general range changes, but this PR was intended to add support for Events only, where the seeking window is clearly defined. But I agree that this could rather easily be extended to also send seek range change events for general live streams, which would put this more in the context of #87 and I'm not sure if that is a good idea for this PR ?!? |
@thasso Sorry about the confusion. Yes indeed, I did use the wrong branch initially but quickly realized and fixed my mistake to find your changes working as advertised. This PR thus provides me with the first ExoPlayer experience I have encountered where some Live behavior works from the player controls perspective, both from showing current time and duration as well as having seek behavior that works. Now I would like to use your changes (in the lack of anything as good or better) to get Live working with a fixed size moving window as illustrated by the iTV24 stream I referenced above. Fwiw, the approach I am using is to generate an available range change event on top of this PR for every new chunk loaded. |
liveEvent |= mediaPlaylist.liveEvent; | ||
durationUs = live || liveEvent ? C.UNKNOWN_TIME_US : mediaPlaylist.durationUs; | ||
|
||
if(!live || liveEvent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is "live" being filtered out here. I would think that both a sliding fixed size live window and a growing sized live window (is this the same as an "Event" type?) would want to notify listeners about the available seek range change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ojw28 @thasso When I remove the conditional at line 630 and then test against the iTV24 live stream, it sort of works correctly. There is an excessive delay in forward seeking but backward seeking works fine. Can either of you make a suggestion where I can find and attempt to fix this seek bug. I have a hunch it is a known, existing issue.
@pajatopmr - Needed the HLS live seeking feature in ExoPlayer, safe to cherry pick these 4 commits now? |
@ckarthikv No, it is not really safe to take these changes yet. The problem is that seeking forward still does not work correctly for the live stream mentioned in an earlier comment on this PR. There is an unacceptably long delay (sometimes measured in minutes) before playback continues from the desired seek position. I am currently working on solving this problem and will report any progress both here and in a separate issue. |
@pajatopmr - Thanks. My requirement is to get the HLS video to always start from the start of the LiveStream rather than the live-edge, which essentially means just one-time-backward-seeking to time 0L. Would these 4 commits help achieve the same? |
@ckarthikv The PR as it is right now will not help with that as it only adds support for "Live Event" streams, not for general live streams and seeking support general live streams. I have another patch locally to enable that, but I am waiting for some feedback from @ojw28 in the PR first, before making it more complicated and tackle multiple problems at the same time. |
@thasso - Is there any way I could access the patch implemented locally? Would be of great help. |
@thasso @ckarthikv If I read the HLS spec correctly (which matches some comments I have read by @ojw28) seeking to 0 should cause a seek to the live edge. This PR handles the seek to live edge for EVENT type accordingly. Seek to the live edge via 0L does not work for live (no EVENT playlist type) but I am working on making this change. @thasso if you have a fix for this I would love to see it. |
The patch is internal only at this moment, but I can try to get it public during this week maybe. Not sure though if I should merge it with the PR or keep it separate, cause it might indeed solve both live and event-live. General note the seek(0) questions: I don't think this is spec specific, but simply how seeking in HLS Live in master/dev is currently implemented. Take a look here and you will see that in case of a live stream, any seek operation will jump to the current live edge. NOTE that this did change in this PR and you will need to seek to the end of the seek range as reported by the events. |
<h1>Rationale:</h1> This commit attempts to resolve the following issues: - get the key changes required to support Live/DVR merged to master by separating them from changes supporting Live/EVENT (PR#848) in the hope that smaller is better in the eyes of the maintainers, - address an @ojw28 concern that the Demo app not be made more complicated with additional features, - the PR comment from @fredx21 that the Live/DVR demo has issues with the seek bar behavior: to fix this requires a more robust Demo app. <h1>Changes to be committed:</h1> demo/src/main/java/com/google/android/exoplayer/demo/Samples.java demo/src/main/java/com/google/android/exoplayer/demo/player/DemoPlayer.java demo/src/main/java/com/google/android/exoplayer/demo/player/HlsRendererBuilder.java library/src/main/java/com/google/android/exoplayer/util/PlayerControl.java - Revert PR google#848 changes. library/src/main/java/com/google/android/exoplayer/hls/HlsChunkSource.java - Prune out PR google#848 changes and ensure that seeking cannot enter the region beyond the live edge (the last three segments in the media playlist).
Hi @ojw28 I've written this for dev-v2 in much simpler fashion - no new entities, nor listeners. You can see the changes here: https://github.com/google/ExoPlayer/compare/dev-v2...jonasevcik:startover?expand=1 |
No updates on this? |
This pull requests adds support for HLS Live Event Streams where
EXT-X-PLAYLIST-TYPE
is set toEVENT
and the player should allow seeking within the currently available range.The seek range change is exposed through the EventListener and a delegation constructor was added so the only API change is the change of the EventListener interface.
(FYI Company Contributor Agreement should be on the way)