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

Make merged segment size configurable #248

Merged
merged 1 commit into from Feb 8, 2023

Conversation

lemondoglol
Copy link

This change is related to this request google/ExoPlayer#10949

For making MAX_MERGED_SEGMENT_START_TIME_DIFF_US configurable

@google-cla
Copy link

google-cla bot commented Jan 30, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@lemondoglol
Copy link
Author

morning @marcbaechinger ! Could you review this when you have time? Thanks so much! (I can't assign this PR to you as a reviewer somehow)

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

@marcbaechinger marcbaechinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks!

I've added my comments. Most are nits. If you want to update your PR accordingly that would be great! Else I'd proceed sooner or later and apply the changes myself before I give into internal review and submit. :)

Thanks for your contribution!

@@ -76,7 +76,8 @@ public int compareTo(Segment other) {
}

private static final int BUFFER_SIZE_BYTES = 128 * 1024;
private static final long MAX_MERGED_SEGMENT_START_TIME_DIFF_US = 20 * C.MICROS_PER_SECOND;
public static final long DEFAULT_MAX_MERGED_SEGMENT_START_TIME_DIFF_US = 20 * C.MICROS_PER_SECOND;
private static long MAX_MERGED_SEGMENT_START_TIME_DIFF_US = DEFAULT_MAX_MERGED_SEGMENT_START_TIME_DIFF_US;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make it a final member field, rename to maxMergedSegmentStartTimeDiffUs and move to the group of private final fields around L89.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxMergedSegmentStartTimeDiffUs can't be final. And if we make it final, then it would give error in constructor where we modify this field.

And it needs to be static, since it is being used in static method (and since it needs to be static, so we would need to give an initial value to it).

Please let me know if I misunderstand your comment

This comment was marked as spam.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I get it. mergeSegments is static.

We still want it as a member field of the downloader. We pass the value into the static method mergeSegments as an additional param then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, so modified the mergeSegments parameters, now it can be non-static and be final

@microkatz microkatz merged commit ecd91d8 into androidx:main Feb 8, 2023
tonihei pushed a commit that referenced this pull request Mar 2, 2023
PiperOrigin-RevId: 507784608
(cherry picked from commit ecd91d8)
rohitjoins pushed a commit that referenced this pull request Mar 16, 2023
PiperOrigin-RevId: 507784608
(cherry picked from commit 08342ea)
tianyif pushed a commit that referenced this pull request Mar 31, 2023
@androidx androidx locked and limited conversation to collaborators Apr 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants