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

Request to open up configurability for Segment download collapsing #10949

Closed
ronak2121 opened this issue Jan 27, 2023 · 12 comments
Closed

Request to open up configurability for Segment download collapsing #10949

ronak2121 opened this issue Jan 27, 2023 · 12 comments
Assignees

Comments

@ronak2121
Copy link

ronak2121 commented Jan 27, 2023

When filing a feature request:

Replace the content in the sections below.

[REQUIRED] Use case description

We are using Exoplayer's Dash Downloader to download our long form audio only content and we see a large degradation in download speeds. Our segment sizes are already set to 20 seconds and our content can be up to 154 hours long.

We saw that Exoplayer already collapses adjacent segments together to optimize the amount of network chattiness on the wire. c8e7ecd. As our segment sizes are already 20s long this collapsing doesn't kick in.

Based on our testing, bumping up this number to something much higher like 2000, or 3000 gives us the speed ups we need.

Proposed solution

We are requesting Exoplayer to make the segment size collapsing configurable via a parameter into the SegmentDownloader instead of a hardcoded value:

private static final long MAX_MERGED_SEGMENT_START_TIME_DIFF_US = 20 * C.MICROS_PER_SECOND;

This can continue to be the default value, but clients can override it with their own values so it works in each of their contexts. (I understand this value was probably selected for video use cases in mind only).

Alternatives considered

Alternatively we can add logic on the server side to dynamically manipulate the segment sizes if Android is trying to download the stream, but this will reduce our cache hit ratios, add special logic to our infrastructure to work around this limitation, and potentially impact stall rates and latency with streaming while we download the stream.

@DevPJ9
Copy link

DevPJ9 commented Jan 27, 2023

Hello @ronak2121 . I would like to work on this.

@ronak2121
Copy link
Author

Thanks for volunteering to help! If you're willing and able to help us submit a PR for this to Exoplayer, let us know how we can help.

@DevPJ9
Copy link

DevPJ9 commented Jan 28, 2023

Thanks @ronak2121 for giving me the opportunity. I am willing to help or open a PR if it's fine.

I think the suggestion you mentioned can be implemented by making a booster (variable name) which will hold value input by user. If it is 0 we can go with the default value MAX_MERGED_SEGMENT_START_TIME_DIFF_US = 20 * C.MICROS_PER_SECOND; else we can assign the user value as you proposed in the solution.

This is my implementation of the solution based on your proposed one. Let me know if I am wrong.

Thank you.

@marcbaechinger marcbaechinger self-assigned this Jan 30, 2023
@ronak2121
Copy link
Author

I yield to @marcbaechinger on the best way to implement this.

@marcbaechinger
Copy link
Contributor

Thanks both. Replacing the constant with a field value of SegmentDownloader that can be customized sounds good to me.

Please file the pull request against the main branch of the androidx/media repository. It will land in both repos once it is through.

Guess this should be straight-forward. Create a field named like the constant but as a field of SegmentDownloader, then use the field instead of the constant in mergeSegements() and add a constructor param that allows to set the value. Rename the constant to DEFAULT_... and make it public so subclasses can use the default.

This then needs to be applied to DashDownloader, HlsDownloader and SsDownloader. I suggest to add the param to the main constructor that has the most parameters. Please deprecate the old constructor and delegate the call with the default value to the new main constructor. Copy JavaDoc from the deprecated to the new constructor.

File the pull request to androidx/media and assign to me.

@ronak2121
Copy link
Author

ronak2121 commented Jan 30, 2023

We're working on a PR. My team mate @lemondog will post it shortly.

@lemondoglol
Copy link

lemondoglol commented Jan 30, 2023

this is the CR androidx/media#248; Please review! Thanks so much!

@ronak2121
Copy link
Author

Hi @marcbaechinger once this is merged, is it possible to release it officially with a new Exoplayer version?

@marcbaechinger
Copy link
Contributor

We are currently in the process of a release (Media3 rc1), but this feature won't make it I'm afraid. It will be part of the stable Media3 release which is probably soon after, but I don't actually know when that will be. The code will be on the main branch of the androidx/media and this repository soon after it is internally submitted (this week).

@ronak2121
Copy link
Author

Hi @marcbaechinger we saw version 2.18.3 came out. I imagine our patch here will be in the next release. Is there a timeline for that?

@marcbaechinger
Copy link
Contributor

Yeah, sorry I was hoping to include it in rc but it didn't made it in. We are already preparing for the next release which will be released in March (2 - 3 weeks time).

@ronak2121
Copy link
Author

Closing this as the issue is resolved and our PR already incorporated in today's release.

@google google locked and limited conversation to collaborators May 6, 2023
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