Support for HLS EXT-X-IFRAME-ONLY playlists by stevemayhew · Pull Request #6270 · google/ExoPlayer

stevemayhew · 2019-08-07T18:27:34Z

This branch adds support for parsing HLS I-Frame only playlists and adding them as a selectable Format in an TrackGroup.

All existing unit test cases pass, the branch includes one test case for iFrame specifically. I modified AdaptiveTrackSelection to not include these IDR only Formats, 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.

googlebot · 2019-08-07T18:27:47Z

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 @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 and verify that your email is set on your git commits.

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 (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

stevemayhew · 2019-08-07T18:31:31Z

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

stevemayhew · 2019-08-07T18:32:19Z

This will prevent any of the Iframe Format from being part of default ABR.

stevemayhew · 2019-08-07T18:33:50Z

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.

stevemayhew · 2019-08-07T18:34:40Z

Alternate 'VIDEO' renditions should be a TODO, it is in the spec but I've never seen it in the wild.

stevemayhew · 2019-08-07T18:40:43Z

ROLE_FLAG_ALTERNATE should probably go, I added this to surface it to the track selector UI.

liambresnahan · 2019-08-07T19:03:50Z

@googlebot I signed it!

stevemayhew · 2019-08-07T21:11:18Z

@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>.

googlebot · 2019-08-07T21:11:23Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

tonihei · 2019-08-19T11:17:34Z

@AquilesCanta Would you mind reviewing this change? See #474 for context.

AfanasievAnton · 2020-03-12T13:57:23Z

hey, was wondering about your future plans with regard to this pr.
Do you consider to review it, or decline. And if so, are you planning to provide some kind of support for I-FRAMES extraction?

stevemayhew · 2020-03-12T17:08:06Z

@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.

ojw28 · 2020-03-12T18:19:11Z

@AquilesCanta - Did you have a chance to look at this?

stevemayhew · 2020-03-17T20:59:33Z

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).

@ojw28

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.

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.

stevemayhew · 2020-03-18T00:22:14Z

Rebased to latest dev-v2 (removing the old merge commits). @AquilesCanta let me know if you need me to rebase and collapse some commits to make it clearer. I did merge in the updates to use the new Format.builder() in the method I refactor extracted.

stevemayhew · 2020-03-18T00:27:50Z

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 AdaptiveTrackSelection and change this code to select i-Frame only track if over 8x speed (you'll want to disable audio if you do this):

  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
  }

AquilesCanta · 2020-03-18T00:35:24Z

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.

stevemayhew · 2020-03-18T01:06:56Z

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.

AquilesCanta

Thanks for the PR! Two main points to visit, rest of comments just nits.

AquilesCanta · 2020-03-19T20:06:16Z

      Format format, int trackBitrate, float playbackSpeed, long effectiveBitrate) {
-    return Math.round(trackBitrate * playbackSpeed) <= effectiveBitrate;
+
+    boolean isNonIframeOnly = (format.roleFlags & C.ROLE_FLAG_TRICK_PLAY) == 0;


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?

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.

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.

AquilesCanta · 2020-03-19T20:12:12Z

-    if (hasVariants) {
+    ArrayList<Variant> allVariants = new ArrayList<>();
+    allVariants.addAll(masterPlaylist.variants);
+    allVariants.addAll(masterPlaylist.iFrameVariants);


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.

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.

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?

Seems to be pretty well contained now. Thanks!

@AquilesCanta

Address the minor issues raised in @AquilesCanta's review of pull request 6270. Also remove unused variable (`hasIFrameVariants`)

stevemayhew · 2020-03-20T17:47:56Z

            : Collections.emptyMap();

    boolean hasVariants = !masterPlaylist.variants.isEmpty();
-    boolean hasIFrameVariants = !masterPlaylist.iFrameVariants.isEmpty();


Variable removed, unused.

AquilesCanta

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!

AquilesCanta · 2020-03-20T12:19:36Z

-    if (hasVariants) {
+    ArrayList<Variant> allVariants = new ArrayList<>();
+    allVariants.addAll(masterPlaylist.variants);
+    allVariants.addAll(masterPlaylist.iFrameVariants);


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?

AquilesCanta · 2020-03-21T16:12:18Z

   * @param baseUri See {@link #baseUri}.
   * @param tags See {@link #tags}.
   * @param variants See {@link #variants}.
+   * @param iFrameVariants


Nit: Can we add See {@link iFrameVariants}?

AquilesCanta · 2020-03-27T18:57:24Z

-    if (hasVariants) {
+    ArrayList<Variant> allVariants = new ArrayList<>();
+    allVariants.addAll(masterPlaylist.variants);
+    allVariants.addAll(masterPlaylist.iFrameVariants);


Seems to be pretty well contained now. Thanks!

AquilesCanta · 2020-06-16T15:57:32Z

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.

stevemayhew · 2020-06-16T16:22:49Z

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>.

AquilesCanta · 2020-06-16T17:15:28Z

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.

AquilesCanta · 2020-06-16T17:21:01Z

Filed #7512.

stevemayhew · 2020-06-16T17:21:42Z

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>.

googlebot added the cla: no label Aug 7, 2019

stevemayhew changed the title ~~P iframe only playlist~~ Support for HLS EXT-X-IFRAME-ONLY playlists Aug 7, 2019

stevemayhew commented Aug 7, 2019

View reviewed changes

Comment thread ...y/hls/src/main/java/com/google/android/exoplayer2/source/hls/playlist/HlsMasterPlaylist.java Outdated

stevemayhew commented Aug 7, 2019

View reviewed changes

Comment thread ...y/hls/src/main/java/com/google/android/exoplayer2/source/hls/playlist/HlsPlaylistParser.java Outdated

stevemayhew commented Aug 7, 2019

View reviewed changes

googlebot added cla: yes and removed cla: no labels Aug 7, 2019

stevemayhew mentioned this pull request Aug 13, 2019

HLS support for trick play with EXT-X-I-FRAMES-ONLY and EXT-X-I-FRAME-STREAM-INF #474

Closed

tonihei assigned AquilesCanta Aug 19, 2019

defagos mentioned this pull request Aug 19, 2019

tvOS support SRGSSR/srgmediaplayer-apple#61

Closed

18 of 21 tasks

stevemayhew added 4 commits March 17, 2020 14:32

IFrame only exposed as a video track.Plays correctly

f076a5e

Add basic unit test for parsing.

33667eb

Default to not select iFrame oly tracks.

64cd438

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.

stevemayhew force-pushed the p-iframe-only-playlist branch from aadb36b to b3796cc Compare March 18, 2020 00:01

stevemayhew force-pushed the p-iframe-only-playlist branch from b3796cc to 1f11233 Compare March 18, 2020 00:18

AquilesCanta reviewed Mar 19, 2020

View reviewed changes

Address the minor issues from pull request review

8c4d31a

Address the minor issues raised in @AquilesCanta's review of pull request 6270. Also remove unused variable (`hasIFrameVariants`)

stevemayhew commented Mar 20, 2020

View reviewed changes

CalebGartner mentioned this pull request Mar 23, 2020

Corrupted playback of 15x I-frame H264 TS content #7131

Closed

AquilesCanta reviewed Mar 27, 2020

View reviewed changes

AquilesCanta added the should merge label Mar 27, 2020

marcbaechinger merged commit 88de774 into google:dev-v2 Apr 17, 2020

google locked and limited conversation to collaborators Jun 23, 2020

stevemayhew deleted the p-iframe-only-playlist branch June 11, 2021 20:40

Conversation

stevemayhew commented Aug 7, 2019

Uh oh!

googlebot commented Aug 7, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liambresnahan commented Aug 7, 2019

Uh oh!

stevemayhew commented Aug 7, 2019 via email

Uh oh!

googlebot commented Aug 7, 2019

Uh oh!

tonihei commented Aug 19, 2019

Uh oh!

AfanasievAnton commented Mar 12, 2020

Uh oh!

stevemayhew commented Mar 12, 2020

Uh oh!

ojw28 commented Mar 12, 2020

Uh oh!

stevemayhew commented Mar 17, 2020

Uh oh!

stevemayhew commented Mar 18, 2020

Uh oh!

stevemayhew commented Mar 18, 2020

Uh oh!

AquilesCanta commented Mar 18, 2020

Uh oh!

stevemayhew commented Mar 18, 2020

Uh oh!

AquilesCanta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevemayhew Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AquilesCanta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevemayhew Mar 19, 2020 •

edited

Loading