Skip to content
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

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

Closed
juechemparathy opened this issue May 25, 2017 · 6 comments
Assignees

Comments

@juechemparathy
Copy link

juechemparathy commented May 25, 2017

Apple spec talks about having average bandwidth and peak bandwidth in master playlist as below -
'You should include the average bit rate as well as the peak bit rate. This is especially important if your peak is more than 20% different from your average. Specifying the average allows the client to make more intelligent decisions about which variant to play.'
Ref -
https://tools.ietf.org/html/draft-pantos-http-live-streaming-23#page-28
https://developer.apple.com/library/content/technotes/tn2224/_index.html#//apple_ref/doc/uid/DTS40009745-CH1-BITRATERECOMMENDATIONS

Exoplayer seems to be expecting only average Bandwidth in the master playlist as the regex in HLSplaylistParser is "BANDWIDTH=(\d+)\b".

For supporting VBR we need a) regex fix to extract average bitrate and peak bitrate, b) data holder in Format object for peak bitrate

Sample snippet from a master .m3u8 file -
#EXT-X-STREAM-INF:PROGRAM-ID=1,BANDWIDTH=2095000,SUBTITLES="subs",AVERAGE-BANDWIDTH=1945000,RESOLUTION=1280x720

I am looking at Exo v2.3.1.
Thanks, _jue

ojw28 pushed a commit that referenced this issue May 31, 2017
Also prevent BANDWIDTH's regex from matching the AVERAGE-BANDWIDTH attribute.

Issue:#2863

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=157219453
@ojw28
Copy link
Contributor

ojw28 commented May 31, 2017

The change above updates ExoPlayer to use AVERAGE-BITRATE to populate Format.bitrate when it's available, since that's what Format.bitrate is documented to contain.

It's somewhat unclear to me that there's a need to also include the peak bitrate in the Format object. Unless people are going to write ABR algorithms that make use of it, the need to include it is pretty hypothetical. If you do have a concrete near term plan to make use of it, please could you clarify how you intend to use it (particularly given you don't know where the peaks are).

@juechemparathy
Copy link
Author

@ojw28 - Yes, we intend to use it in our custom ABR algorithm(esp with fmp4) and is also considered as a useful input in our VBR instrumentation data. Thanks.

@ojw28 ojw28 changed the title Not consuming peak bitrate in master playlist to enable VBR support Expose peak bitrate in Format when available (as well as average bitrate) Jun 6, 2017
ojw28 pushed a commit that referenced this issue Jun 6, 2017
Also prevent BANDWIDTH's regex from matching the AVERAGE-BANDWIDTH attribute.

Issue:#2863

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=157219453
@NayaneshGupte
Copy link

@ojw28 any idea when is this planned tentatively ?

julioz pushed a commit to julioz/ExoPlayer that referenced this issue Nov 27, 2019
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
@ojw28
Copy link
Contributor

ojw28 commented Jan 9, 2020

@julioz (and anyone else who's interested) - I've been looking at how to best merge some version of #6706. It's clear we need two (possibly three to avoid changing behavior in a subtle way) fields in Format. However we need to decide what those fields should be called, what they should be set to if one or the other is not present in the media, and how to map values specified in HLS, SmoothStreaming, DASH and regular media container formats into them. The definitions used in the various specifications are:

HLS

  • AVERAGE-BANDWIDTH - The value is a decimal-integer of bits per second. It represents the average segment bit rate of the Variant Stream.
  • BANDWIDTH - The value is a decimal-integer of bits per second. It represents the peak segment bit rate of the Variant Stream.

DASH

  • @bandwidth - 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).

SmoothStreaming

  • Bitrate - The average bandwidth that is consumed by the track, in bits per second (bps). The value 0 MAY be used for tracks whose bit rate is negligible relative to other tracks in the presentation

Other containers

  • Unknown and possibly container specific, but I would guess they're normally average bit-rates.

Some thoughts:

  • The DASH one is interesting. It's actually very nicely defined, from the perspective of allowing a player to do the correct thing. My interpretation is that it's closer to being a peak bit-rate than an average bit-rate. If @minBufferTime = 0 then it's definitely a peak bit-rate. As @minBufferTime becomes larger it's less of a peak bit-rate, but we'd normally expect it to be set to something relatively small.
  • My interpretation of the HLS BANDWIDTH is that it's a peak bitrate as averaged over a segment. If @minBufferTime is around one segment duration in DASH, then I would guess they end up being approximately equivalent to one another.

Proposal:

  • Keep the current bitrate field as it is today, to avoid breaking anything (possibly to be deprecated)
  • Add averageBitrate, which is populated with AVERAGE-BANDWIDTH in HLS, nothing in DASH, Bitrate in SmoothStreaming, and any values parsed from other container formats (unless we know otherwise - we can set the right one in each case). In the case a value isn't present, Format.NO_VALUE will be used.
  • Add peakBitrate, which is populated with BANDWIDTH in HLS, @bandwidth in DASH, and nothing in SmoothStreaming and for other container formats (unless we know otherwise - we can set the right one in each case). In the case a value isn't present, Format.NO_VALUE will be used.

One caveat here is that code implementing adaptive algorithms will need to be careful to fallback to using the other field in the case that the one it would use ideally is set to Format.NO_VALUE. This is definitely necessary for any adaptive algorithm that wants to support both DASH and SmoothStreaming, because they set different fields. Perhaps this is fine (good?) though, since an adaptive algorithm should probably behave slightly differently depending on whether it's using a peak or average (i.e. if it's using an average then it should be more cautious).

@julioz - Any thoughts on this proposal?

@julioz
Copy link
Contributor

julioz commented Jan 16, 2020

@ojw28 The proposal sounds very reasonable to me.

It does introduce a small extra bit through the learning curve of the API: I assume most consumers read bitrate and aren't interested in whether it's an average or peak, so if we decide to @deprecate that field, we must make sure we point people to either this documentation or some other clarification of the semantic differences.

(I did spend a few moments pondering whether we should instead keep bitrate as a convenience for those callers, but I guess that could cause more confusion that aid; so I believe deprecating it is the way to go.)

kim-vde pushed a commit that referenced this issue Feb 11, 2020
The functional change is to set the bitrate fields to Format.NO_VALUE
in the case that they're non-positive in the header data. Else it's
very easy to to take the fields and copy them directly into Format as
incorrect values.

Issue #2863

PiperOrigin-RevId: 292920141
ojw28 added a commit that referenced this issue Feb 13, 2020
ojw28 added a commit that referenced this issue Feb 13, 2020
Issue: #2863
PiperOrigin-RevId: 294661214
ojw28 added a commit that referenced this issue Feb 25, 2020
Package private for now. It will be made visible in a child CL.

Issue: #2863
PiperOrigin-RevId: 296255558
ojw28 added a commit that referenced this issue Feb 25, 2020
- Deprecate old Format.createXXX methods
- Deprecate most Format.copyXXX methods
- Stop using deprecated Format.copyXXX methods in the library

Note: Replacing library usages of Format.createXXX method
will be done in follow up CLs. These changes aren't purely
mechanical because we need to decide which out of peakBitrate
and averageBitrate to set in each case where currently a
single bitrate is provided.

Issue: #2863
PiperOrigin-RevId: 296450935
ojw28 added a commit that referenced this issue Feb 25, 2020
Issue: #2863
PiperOrigin-RevId: 296482726
@ojw28 ojw28 closed this as completed Feb 28, 2020
@ojw28
Copy link
Contributor

ojw28 commented Feb 28, 2020

This is done in dev-v2. There is also a Format.Builder now, to make future changes to Format less painful.

@google google locked and limited conversation to collaborators Apr 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants