Expose `peakBitrate` property in `Format` #6706

julioz · 2019-11-27T21:03:30Z

Exposes peakBitrate value when both BANDWIDTH and AVERAGE-BANDWIDTH attributes are available.
The bitrate property will always carry the average bandwidth of the Format instance, while peakBitrate
will carry the value for the respective attribute, when present; according to
tools.ietf.org/html/draft-pantos-http-live-streaming-23#page-28

Fixes google#2863 Exposes `peakBitrate` value when both `BANDWIDTH` and `AVERAGE-BANDWIDTH` attributes are available. The `bitrate` property will always carry the average bandwidth of the `Format` instance, while `peakBitrate` will carry the value for the respective attribute, when present; according to tools.ietf.org/html/draft-pantos-http-live-streaming-23#page-28

julioz · 2019-11-27T21:05:03Z

...test/java/com/google/android/exoplayer2/source/hls/playlist/HlsMasterPlaylistParserTest.java

    assertThat(variants.get(1).format.bitrate).isEqualTo(1270000);
+    assertThat(variants.get(1).format.peakBitrate).isEqualTo(1280000);


This is the example case for the presence of the attribute.

julioz · 2019-11-27T21:05:14Z

...test/java/com/google/android/exoplayer2/source/hls/playlist/HlsMasterPlaylistParserTest.java

@@ -236,14 +236,17 @@ public void testParseMasterPlaylist() throws IOException {
  }

  @Test
-  public void testMasterPlaylistWithBandwdithAverage() throws IOException {
+  public void testMasterPlaylistWithBandwidthAverage() throws IOException {


Fixed typo.

julioz · 2019-11-27T21:06:59Z

When running locally, some Extractor tests fail; I can't figure out why. I assume it's one of the (innumerous) Format constructors (or factory methods) that take in bitrate and didn't take in peakBitrate. Please advise before we merge this (or; feel free to fix and commit on top of this branch).

icbaker · 2019-11-29T18:12:37Z

Mind taking a look Santiago?

AquilesCanta · 2019-12-02T10:05:31Z

I think this is for @ojw28 to look at.

AquilesCanta · 2019-12-02T10:08:27Z

On my side, I think it's unfortunate we are using the Format.bitrate field for both BANDWIDTH and AVERAGE-BANDWIDTH. Adding the peak bitrate adds a bit of confusion to the mix.

Aside, I think it would be helpful for us to know why/how having both average and peak bitrates could be of use.

julioz · 2019-12-02T10:16:29Z

@AquilesCanta Please check the referred issue #2863 for context.

AquilesCanta · 2019-12-02T10:19:48Z

My bad, didn't see the link. However, the referenced issue makes the same question I made above. And an answer is still not available. It's worth looking into the issue first.

EDIT: @ojw28 removed the need more info tag from the issue, so I'm assuming he understands the use case. Thanks for your patience!

tonihei · 2019-12-02T10:28:30Z

I think the peak bitrate is preferable for adaptive track selection to be on the safe side, especially when the peak is much higher than the average. Otherwise, the ABR algorithm may run into issues during complex scenes.

So, if available, we should probably just populate the Format bitrate by BANDWIDTH in all cases?

julioz · 2019-12-02T10:33:36Z

@tonihei That is for sure. The BANDWIDTH attribute is mandatory, while the AVERAGE-BANDWIDTH attribute is optional.
The change in 2795269 exposes the AVERAGE-BANDWIDTH value overriding the BANDWIDTH one, but that means we hide away the latter. For advanced ABR algorithms that might need to introspect into that value, this becomes an issue; which this PR fixes.

Let me know if hiding one of the values is by design. If that's the case, we should document it publicly somewhere, I assume others might hit the same limitation in the future.

AquilesCanta · 2019-12-02T10:35:10Z

To provide some context: Back when AVERAGE-BANDWIDTH was added to the HLS spec, preferring AVERAGE-BANDWIDTH (which is option) over BANDWIDTH (which is mandatory) was a design decision. @tonihei, please feel free to take whichever approach you think is most suitable for ABR.

ojw28 · 2019-12-02T17:06:25Z

I think the peak bitrate is preferable for adaptive track selection to be on the safe side, especially when the peak is much higher than the average. Otherwise, the ABR algorithm may run into issues during complex scenes.

Presumably using both is preferable, else there wouldn't be value in them both existing. I agree that if we were to use just one, then peak bitrate is preferable, and so 53b52e7 was probably a mistake.

I think we should expose both in Format, and use peak bitrate for ABR decisions for as long as we're only using one of them. I can sort this out whilst merging this pull request. I will separately propose a small fix to use peak bitrate ahead of merging the whole change.

ojw28 · 2019-12-02T17:36:10Z

For what it's worth, the DASH specification gives a more nuanced definition of bitrate, that seems much more useful than either "peak" or "average". I guess it's closer to "peak" in practice:

Consider a hypothetical constant bitrate channel of bandwidth with the value of this attribute in bits per second (bps). Then, if the Representation is continuously delivered at this bitrate, starting at any SAP that is indicated either by @startWithSAP or by any Segment Index box, a client can be assured of having enough data for continuous playout providing playout begins after @minBufferTime * @bandwidth bits have been received (i.e. at time @minBufferTime after the first bit is received).

Meanwhile the HLS authoring specification seems to allow you to specify average and peak bitrates that are wrong by up to 10%, whilst referencing another section that doesn't exist. Why?

1.26. For VOD content, the average segment bit rate MUST be within 10% of the AVERAGE-BANDWIDTH attribute. (See Declared versus measured values of bandwidths.)
1.27. For VOD content, the measured peak bit rate MUST be within 10% of the BANDWIDTH attribute.

This is a minor change ahead of merging a full variant of #6706, to make re-buffers less likely. Also remove variable substitution when parsing AVERAGE-BANDWIDTH (it's not required for integer attributes) PiperOrigin-RevId: 283554106

julioz · 2019-12-16T18:29:20Z

@ojw28 @marcbaechinger It seems like we have opted for exposing the peak bitrate when available, super-seeding the avg bitrate data, which hinders more advanced ABR algorithms.
As this PR starts to get conflicts with dev-v2, I'm tempted to close it. As a side-note, it might be worth extracting some of the constructor calls with default values from many test suites into a test fixture class that can provide sensible values and still builders for further specialization if need be in tests. I didn't do it as part of this PR since I couldn't find other fixture classes in the codebase, but would gladly introduce this pattern as it has proven to be useful for us at SoundCloud.

Let me know what you think.

ojw28 · 2019-12-16T18:41:35Z

Please leave it open. We'll resolve the conflict when we merge. Thanks!

ojw28 · 2020-02-03T18:31:59Z

We need to make some bigger changes to land this, which we're working on now. These changes will land separately of this pull request, so I'll go ahead and close it. Please follow the tracking bug for updates. Thanks!

googlebot added the cla: yes label Nov 27, 2019

julioz commented Nov 27, 2019

View reviewed changes

icbaker requested a review from AquilesCanta November 29, 2019 18:12

icbaker assigned AquilesCanta Nov 29, 2019

AquilesCanta assigned ojw28 and unassigned AquilesCanta Dec 2, 2019

ojw28 added the should merge label Dec 17, 2019

julioz mentioned this pull request Jan 3, 2020

[RFC] Introduce Kotlin #6826

Closed

ojw28 mentioned this pull request Jan 9, 2020

Expose peak bitrate in Format when available (as well as average bitrate) #2863

Closed

ojw28 closed this Feb 3, 2020

ojw28 removed the should merge label Feb 22, 2020

google locked and limited conversation to collaborators Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `peakBitrate` property in `Format` #6706

Expose `peakBitrate` property in `Format` #6706

julioz commented Nov 27, 2019

julioz Nov 27, 2019

julioz Nov 27, 2019

julioz commented Nov 27, 2019 •

edited

Loading

icbaker commented Nov 29, 2019

AquilesCanta commented Dec 2, 2019

AquilesCanta commented Dec 2, 2019

julioz commented Dec 2, 2019

AquilesCanta commented Dec 2, 2019 •

edited

Loading

tonihei commented Dec 2, 2019

julioz commented Dec 2, 2019 •

edited

Loading

AquilesCanta commented Dec 2, 2019

ojw28 commented Dec 2, 2019

ojw28 commented Dec 2, 2019 •

edited

Loading

julioz commented Dec 16, 2019

ojw28 commented Dec 16, 2019

ojw28 commented Feb 3, 2020

		assertThat(variants.get(1).format.bitrate).isEqualTo(1270000);
		assertThat(variants.get(1).format.peakBitrate).isEqualTo(1280000);

Expose peakBitrate property in Format #6706

Expose peakBitrate property in Format #6706

Conversation

julioz commented Nov 27, 2019

julioz Nov 27, 2019

Choose a reason for hiding this comment

julioz Nov 27, 2019

Choose a reason for hiding this comment

julioz commented Nov 27, 2019 • edited Loading

icbaker commented Nov 29, 2019

AquilesCanta commented Dec 2, 2019

AquilesCanta commented Dec 2, 2019

julioz commented Dec 2, 2019

AquilesCanta commented Dec 2, 2019 • edited Loading

tonihei commented Dec 2, 2019

julioz commented Dec 2, 2019 • edited Loading

AquilesCanta commented Dec 2, 2019

ojw28 commented Dec 2, 2019

ojw28 commented Dec 2, 2019 • edited Loading

julioz commented Dec 16, 2019

ojw28 commented Dec 16, 2019

ojw28 commented Feb 3, 2020

Expose `peakBitrate` property in `Format` #6706

Expose `peakBitrate` property in `Format` #6706

julioz commented Nov 27, 2019 •

edited

Loading

AquilesCanta commented Dec 2, 2019 •

edited

Loading

julioz commented Dec 2, 2019 •

edited

Loading

ojw28 commented Dec 2, 2019 •

edited

Loading