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

Allow unsigned integers in adaptation set id #487

Conversation

vishnuchilakala
Copy link
Contributor

@vishnuchilakala vishnuchilakala commented Jun 28, 2023

Fixes #485

@tonihei tonihei force-pushed the allow_unsigned_int_for_adaptation_set_id branch from 114cf04 to 58baeeb Compare June 29, 2023 09:27
@tonihei tonihei self-assigned this Jun 29, 2023
This avoids issues with potential number clashes with ID_UNSET.
Also fixes some further parsing where adaptation set ids are referenced
from manifest properties.
@tonihei tonihei force-pushed the allow_unsigned_int_for_adaptation_set_id branch from 729311f to 4169386 Compare June 30, 2023 07:44
@tonihei
Copy link
Collaborator

tonihei commented Jun 30, 2023

After some further discussion during internal review, we switched the parsing to long after all (that is, full 64-bit integer). Please have a look at the additional changes I uploaded to this PR. This helps to avoid the negative parsed numbers, which may be confusing. Also, Integer.parseUnsignedInt turned out not to be available on all Android versions we support.

@vishnuchilakala
Copy link
Contributor Author

@tonihei thanks for making the changes. looks good to me 👍

@vishnuchilakala
Copy link
Contributor Author

@tonihei are we good with these changes? If you need any further information from my side, please do let me know.

@tonihei
Copy link
Collaborator

tonihei commented Jul 4, 2023

Yes, it's all merged in our internal codebase and you'll see it merged here too the next time we update our changes (~once a week).

@vishnuchilakala
Copy link
Contributor Author

vishnuchilakala commented Jul 5, 2023

@tonihei Please help us understand the Media3 vs Exoplayer situation.

We are still on the old Exoplayer and the migration is not planned any time soon because of some other dependencies. So how would we proceed here?

Would all the changes that are being contributed to Media3 also be ported to Exoplayer, and vice versa? How long would the team continue supporting the old Exoplayer? Is there any difference in the timelines that the changes would be released, between the two? Would appreciate any information that you can share with us to handle this situation. Thanks!

@tonihei
Copy link
Collaborator

tonihei commented Jul 5, 2023

The code base is 100% identical and we push the same code to both repositories (just that we rewrite package names for ExoPlayer in the process).

We are still on the old Exoplayer and the migration is not planned any time soon because of some other dependencies.

I'm curious about this part. What kind of dependency prevents you from migrating? The migration step for your code should be very straight-forward with the migration script that does everything for you. As pointed out above, the code is fully identical, so this is really just about renaming package imports.

the timelines that the changes would be released,

Given this is a bug fix, it will be part of any Media3 1.1.1 release and ExoPlayer 2.19.1 (which will be identical again). There is no guarantee we actually do such a bug fix release, but it's not too unlikely because there will always be a few things worth fixing in a release.

We will also continue to push changes to both repositories for a while, but we are not planning to release ExoPlayer 2.20 (matching Media3 1.2.0).

@vishnuchilakala
Copy link
Contributor Author

What kind of dependency prevents you from migrating?

We depend on some third parties for their proprietary analytics and SSAI, who we work with using Exoplayer components. They are not planning to migrate yet, effectively blocking us too from doing it.

it will be part of any Media3 1.1.1 release and ExoPlayer 2.19.1

I just saw that 2.19.0 has been released and everything has been marked as deprecated. So can you please confirm that we will get another minor release that will have this change?

We will also continue to push changes to both repositories for a while

We are trying to understand your timelines so that we can bring those third parties to the table and plan the migrations accordingly. Would it be fair to assume that any fixes / enhancements you make for another month will be released on the old Exoplayer also?

Appreciate the help, thanks!

@tonihei
Copy link
Collaborator

tonihei commented Jul 5, 2023

They are not planning to migrate yet, effectively blocking us too from doing it.

That's interesting. But I guess it's the same as them not updating from ExoPlayer version 2.x to 2.y, meaning you are blocked on 2.x?

So can you please confirm that we will get another minor release that will have this change?

I can't really confirm it, but we usually wait a bit to see what kind of bug fixes we accumulate and make a new bug fix release if needed. We've almost always done one in the past, so I'm fairly sure we end up doing Media3 1.1.1. and ExoPlayer 2.19.1 too. Your change isn't included in the main 2.19.0 yet because it happened after the internal branch cut off.

Would it be fair to assume that any fixes / enhancements you make for another month will be released on the old Exoplayer also?

We just push to the dev-v2 branch. We are not planning any further ExoPlayer releases except for 2.19.x bugfix releases (which are all aligned with the corresponding Media3 1.1.x bugfix release).

@microkatz microkatz merged commit 9513f2c into androidx:main Jul 5, 2023
@vishnuchilakala
Copy link
Contributor Author

But I guess it's the same as them not updating from ExoPlayer version 2.x to 2.y, meaning you are blocked on 2.x?

Yep, that's basically it.

Thanks a lot for the information @tonihei ! This is really helpful for us to plan our roadmap.

Copy link

@Banks666 Banks666 left a comment

Choose a reason for hiding this comment

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

Dd

tianyif pushed a commit that referenced this pull request Aug 14, 2023
…daptation_set_id

PiperOrigin-RevId: 544601945
(cherry picked from commit 9513f2c)
@vishnuchilakala vishnuchilakala deleted the allow_unsigned_int_for_adaptation_set_id branch August 31, 2023 06:01
@androidx androidx locked and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants