-
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
Support for HLS EXT-X-IFRAME-ONLY playlists #6270
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
// TODO - not a 'role' in the sense it is parsed from the CHARACTERISTICS attribute... forced if iFrame only | ||
/** Indicates the track is an IDR (IFrame) only track for trick play */ | ||
public static final int ROLE_FLAG_TRICK_PLAY = 1 << 14; | ||
|
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.
I'm certainly not married to this idea... But, there needs to be some hook in the Format
to identify the Iframe only tracks. It's unfortunate the HLS spec does not mandate (or even allow) specifying the frame rate on the Master Playlist EXT-X-IFRAME-ONLY
boolean isNonIframeOnly = (format.roleFlags & C.ROLE_FLAG_TRICK_PLAY) == 0; | ||
boolean canSelect = Math.round(trackBitrate * playbackSpeed) <= effectiveBitrate; | ||
|
||
return canSelect && isNonIframeOnly; // Default is not to use the IDR only tracks in selection | ||
} |
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.
This will prevent any of the Iframe Format
from being part of default ABR.
buildAndPrepareMainSampleStreamWrapper( | ||
masterPlaylist, | ||
positionUs, | ||
sampleStreamWrappers, | ||
manifestUrlIndicesPerWrapper, | ||
overridingDrmInitData); | ||
overridingDrmInitData, | ||
allVariants); | ||
} |
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.
Sorry the diff might be a bit hard to read. Basically added an argument to pass Variants and replaced all references to the variants
field in the HlsMasterPlaylist
with same.
/* subtitleGroupId */null, | ||
/* captionGroupId */null); | ||
} | ||
|
||
/** |
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.
Alternate 'VIDEO' renditions should be a TODO, it is in the spec but I've never seen it in the wild.
...y/hls/src/main/java/com/google/android/exoplayer2/source/hls/playlist/HlsMasterPlaylist.java
Show resolved
Hide resolved
...y/hls/src/main/java/com/google/android/exoplayer2/source/hls/playlist/HlsPlaylistParser.java
Show resolved
Hide resolved
int bitrate = parseStreamBitrate(variableDefinitions, line); | ||
Format format = parseVideoContainerFormat(variableDefinitions, line, formatId, bitrate, | ||
C.ROLE_FLAG_ALTERNATE | C.ROLE_FLAG_TRICK_PLAY); | ||
|
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.
ROLE_FLAG_ALTERNATE should probably go, I added this to surface it to the track selector UI.
@googlebot I signed it! |
@googlebot I signed it!
… On Aug 7, 2019, at 11:28 AM, googlebot ***@***.***> wrote:
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
📝 Please visit https://cla.developers.google.com/ <https://cla.developers.google.com/> to sign.
Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.
What to do if you already signed the CLA
Individual signers
It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>.
Corporate signers
Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot <http://go/cla#troubleshoot> (Public version <https://opensource.google.com/docs/cla/#troubleshoot>).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>.
The email used to register you as an authorized contributor must also be attached to your GitHub account <https://github.com/settings/emails>.
ℹ️ Googlers: Go here <https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fgoogle%2FExoPlayer%2Fpull%2F6270> for more info.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#6270?email_source=notifications&email_token=AADBF6AJFLLZOXLNQTQQOE3QDMH57A5CNFSM4IKDDJM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3ZJWLY#issuecomment-519215919>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBF6A6OA6YSHEIX3OLFXDQDMH57ANCNFSM4IKDDJMQ>.
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@AquilesCanta Would you mind reviewing this change? See #474 for context. |
hey, was wondering about your future plans with regard to this pr. |
@AfanasievAnton I've got another pull reques that also cover this same set of files, but I need to rebase this one. Will do that this week. We are using this with our release version of ExoPlayer (2.10.6 based) for visual trick play. |
@AquilesCanta - Did you have a chance to look at this? |
Added a commit to enable support for "implicit" Media Initialization Section (PAT/PMT for MPEG-TS). This is generated by Apple's tools (mediafilesegmenter for example). |
This was a change suggested by @ojw28 to allow adpative track selection to work with IFrame only `Variant` as any other adaptive seleted `Track` in the `TrackGroup`. This actually works quite well with the exceptions that: 1. IFrame only tracks do not contain the same Rendition Group references as other non-iframe only varaints (so track selection should disable these tracks even in other non-video renderers. 2. Adapting to iFrame tracks only based on bandwidth might not be so good (as often larger spacial resolution iFrame track might have higher bitrate then lower spacial resolution non-iframe only track). This change includes one proposed workaround in the AdaptiveTrackSelection class (only use iFrame for higher playback speeds).
Pull in change to default to not use i-Frames for base AdaptiveTrackSelection. The expectation is a subclases, that support transitioning to iFrame, will select these tracks.
aadb36b
to
b3796cc
Compare
In the HLS Spec (https://tools.ietf.org/html/draft-pantos-hls-rfc8216bis-04#section-4.4.3.6), specifically this or condition of this statement: "... defined by the EXT-X-MAP tag, or is located between the start of the resource and the offset of the first I-frame segment in that resource." Change adds code to add this "implicit" Media Initialization Segment if no EXT-X-MAP defines one explicitly.
b3796cc
to
1f11233
Compare
Rebased to latest |
PS I can send you some sample content to try as well. You can either modify demo player to change speed or simply use track selector to override select the i-Frame only track and play it. Locally I subclassed protected boolean canSelectFormat(
Format format, int trackBitrate, float playbackSpeed, long effectiveBitrate) {
boolean isNonIframeOnly = (format.roleFlags & C.ROLE_FLAG_TRICK_PLAY) == 0;
boolean canSelect = Math.round(trackBitrate * playbackSpeed) <= effectiveBitrate;
return canSelect && isNonIframeOnly; // Default is not to use the IDR only tracks in selection
} |
If this is now updated to dev-v2 I'll do my best to look into it this week. Sorry for the delay, there are some high priority deadlines. |
No worries @AquilesCanta I'm becoming good at merging it ;-) I'm keeping a back-ported to 2.10.6 version as well for us internally where I've added the support for the implicit init segment (Apple's hack), so that triggered updating this branch too. I'll keep using rebase to avoid pollution of the history with merges. |
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.
Thanks for the PR! Two main points to visit, rest of comments just nits.
@@ -540,7 +540,11 @@ public int evaluateQueueSize(long playbackPositionUs, List<? extends MediaChunk> | |||
@SuppressWarnings("unused") | |||
protected boolean canSelectFormat( | |||
Format format, int trackBitrate, float playbackSpeed, long effectiveBitrate) { | |||
return Math.round(trackBitrate * playbackSpeed) <= effectiveBitrate; | |||
|
|||
boolean isNonIframeOnly = (format.roleFlags & C.ROLE_FLAG_TRICK_PLAY) == 0; |
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.
Nit: Can we replace this variable is isTrickPlayTrack or isIFrameOnlyTrack (would require negating the comparison to != 0). Otherwise, it could be read as a track that has no I-frames, which would be inaccurate. Right?
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.
Sure, that sounds fine. Really this logic is just to prevent selecting these tracks ever, for now. The right thing to do is add the logic to TrackSelection itself. In our back-port to release we have subclassed AdaptiveTrackSelection
. The idea is when you move to high-speed mode (or scrub based trick-play) a TrackSelection occurs that results in a TrackSelection.Definition
that includes only formats that are relevant (either all i-Frame or all non-iFrame tracks). Oliver had other thoughts about adapting based on frame rate. I think all of these are worth visiting, and the changes would be restricted to track selection so this code could be merged in first.
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.
Replaced with a positive logic variable, hopefully this is more clear. The function should return fall if the track is an iFrame only track.
Logic to return true will depend on the trick-play implementation.
if (hasVariants) { | ||
ArrayList<Variant> allVariants = new ArrayList<>(); | ||
allVariants.addAll(masterPlaylist.variants); | ||
allVariants.addAll(masterPlaylist.iFrameVariants); |
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.
So my view on the I-FRAME tracks is that they should belong in their own track group, I think.
One can summarise (and simplify) a TrackGroup as a group of tracks that can be adapted between.
- However, it's not intended that a player adapts between a regular track and a I-FRAME only tag.
- Additionally, I can conceive a scenario where I-frame tags are track-selected to a different renderer than the variants. For these two reasons I think we should put I-FRAME only tracks in their own track group. To do this, they need to be in their own HlsSampleStreamWrapper.
Please do feel free to provide evidence pointing in a different direction.
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.
I originally did it this way, #474 (comment)
@ojw28 Made good points for including all the tracks in the same adaption set #474 (comment) So basically we are adapting based on frame rate as well as bandwidth.
One advantage to the current approach, that Oliver advocated, is it may make more sense to trick play a very low bandwidths 30FPS track rather than a higher resolution low frame rate i-Frame only track. Having the tracks all in an adaptive set allows this to occur when adapting to speed changes without the overhead of a full on track selection.
I can go either way, could we get the parsing part in first at least, and do this as a later enhancement? Rebasing my collection of pull requests that alter this code is becoming laborious ;-), there is a lot of change to these files.
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.
could we get the parsing part in first at least
Hah, I was going to suggest splitting this into smaller independent pieces, but I didn't want to get too annoying. So I'll assume the scope of this change is only parsing as of now?
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.
Seems to be pretty well contained now. Thanks!
library/hls/src/main/java/com/google/android/exoplayer2/source/hls/HlsMediaPeriod.java
Outdated
Show resolved
Hide resolved
library/hls/src/main/java/com/google/android/exoplayer2/source/hls/HlsMediaPeriod.java
Outdated
Show resolved
Hide resolved
...y/hls/src/main/java/com/google/android/exoplayer2/source/hls/playlist/HlsPlaylistParser.java
Show resolved
Hide resolved
Address the minor issues raised in @AquilesCanta's review of pull request 6270. Also remove unused variable (`hasIFrameVariants`)
@@ -470,7 +470,6 @@ private void buildAndPrepareSampleStreamWrappers(long positionUs) { | |||
: Collections.emptyMap(); | |||
|
|||
boolean hasVariants = !masterPlaylist.variants.isEmpty(); | |||
boolean hasIFrameVariants = !masterPlaylist.iFrameVariants.isEmpty(); |
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.
Variable removed, unused.
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.
Sorry I forgot to send the previous round of comments. I'll try to merge this next week and address any minor changes manually. Thanks for the pull request!
if (hasVariants) { | ||
ArrayList<Variant> allVariants = new ArrayList<>(); | ||
allVariants.addAll(masterPlaylist.variants); | ||
allVariants.addAll(masterPlaylist.iFrameVariants); |
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.
could we get the parsing part in first at least
Hah, I was going to suggest splitting this into smaller independent pieces, but I didn't want to get too annoying. So I'll assume the scope of this change is only parsing as of now?
...y/hls/src/main/java/com/google/android/exoplayer2/source/hls/playlist/HlsMasterPlaylist.java
Show resolved
Hide resolved
...y/hls/src/main/java/com/google/android/exoplayer2/source/hls/playlist/HlsPlaylistParser.java
Show resolved
Hide resolved
...y/hls/src/main/java/com/google/android/exoplayer2/source/hls/playlist/HlsPlaylistParser.java
Show resolved
Hide resolved
@@ -181,6 +206,7 @@ public Rendition(@Nullable Uri url, Format format, String groupId, String name) | |||
* @param baseUri See {@link #baseUri}. | |||
* @param tags See {@link #tags}. | |||
* @param variants See {@link #variants}. | |||
* @param iFrameVariants |
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.
Nit: Can we add See {@link iFrameVariants}?
if (hasVariants) { | ||
ArrayList<Variant> allVariants = new ArrayList<>(); | ||
allVariants.addAll(masterPlaylist.variants); | ||
allVariants.addAll(masterPlaylist.iFrameVariants); |
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.
Seems to be pretty well contained now. Thanks!
...y/hls/src/main/java/com/google/android/exoplayer2/source/hls/playlist/HlsPlaylistParser.java
Show resolved
Hide resolved
Hi @stevemayhew, have you been able to play an i-frame-only variant? I was playing around with this, and it would seem that the fmp4 extractor throws whenever you try to play an i-frame only variant. Could you please confirm for me whether you have used the fmp4 extractor to play i-frame only variants? If not, I think I will disable them until we manage to make it work. |
We are definitely using fMP4 with HLS (looks like @ojw28 is working on adding DASH support, thanks so much, we’ll need that next). I can provide you with a test stream if you’d like from our origin servers.
Perhaps you are testing against the Apple bib-bop streams, I just tried it (with an r2.11.4 build) and it hit an assert in the extractor, can I assign a bug to us and fix it?
… On Jun 16, 2020, at 8:57 AM, Santiago Seifert ***@***.***> wrote:
Hi @stevemayhew <https://github.com/stevemayhew>, have you been able to play an i-frame-only variant?
I was playing around with this, and it would seem that the fmp4 extractor throws whenever you try to play an i-frame only variant. Could you please confirm for me whether you have used the fmp4 extractor to play i-frame only variants? If not, I think I will disable them until we manage to make it work.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#6270 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBF6G5RLHFQ2MSPAWAA6DRW6I7ZANCNFSM4IKDDJMQ>.
|
Correct. I was assuming the issue was that the I-frame only variants were just trimming samples from the original content, so the extractor was expecting the missing samples. So if you have working I-FRAME only streams, do you use a i-frame-only-specific EXT-X-MAP for them (as opposed to just linking to ranges in the original content)? I'll file an issue for fixing I-FRAME only support in the HLS bip-bop streams. You are welcome to send a pull request for fixing that. |
Filed #7512. |
Thanks,
Yes, we’ll have a look at the bug and work on a fix.
All the origins we have support live, the produce the iFrame tracks from the original segments but it is not the same mp4. Apple’s mediafilesegmenter (VOD) simply identifies IDR’s in the original content as byte-ranges from the main mp4.
… On Jun 16, 2020, at 10:15 AM, Santiago Seifert ***@***.***> wrote:
Perhaps you are testing against the Apple bib-bop streams
Correct. I was assuming the issue was that the I-frame only variants were just trimming samples from the original content, so the extractor was expecting the missing samples.
So if you have working I-FRAME only streams, do you use a i-frame-only-specific EXT-X-MAP for them (as opposed to just linking to ranges in the original content)?
I'll file an issue for fixing I-FRAME only support in the HLS bip-bop streams. You are welcome to send a pull request for fixing that.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#6270 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBF6CWG5XQVKDNJZQH2HLRW6SD5ANCNFSM4IKDDJMQ>.
|
This branch adds support for parsing HLS I-Frame only playlists and adding them as a selectable
Format
in anTrackGroup
.All existing unit test cases pass, the branch includes one test case for iFrame specifically. I modified
AdaptiveTrackSelection
to not include these IDR onlyFormats
, so they will not affect current ExoPlayer playback for existing master playlists that already contain iFrame only variants. The expectation is a subclass of same would do this intelligently based on desired playback speed.I can provide (privately) sample broadcast and VOD streams that have IDR streams you can manually select to see the behavior. The existing apple streams all parse and present their iFrame only tracks (bipbop), however playback of these is not very interesting ;-).
See my notes in the code in the individual commits for more thoughts on direction.
PS, our CLA is signed and in Google's hands for approval.