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

Manual updates 20241112 androidx.media3.exoplayer fixes #1075

Merged
merged 14 commits into from
Jan 18, 2025

Conversation

moljac
Copy link
Contributor

@moljac moljac commented Jan 14, 2025

Fixes for

#1061

#985

#954

@moljac
Copy link
Contributor Author

moljac commented Jan 14, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@moljac
Copy link
Contributor Author

moljac commented Jan 14, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 5 changed files in this pull request and generated no comments.

Files not reviewed (3)
  • source/androidx.media3/media3-exoplayer-hls/Transforms/Metadata.xml: Language not supported
  • source/androidx.media3/media3-exoplayer-rtsp/Transforms/Metadata.xml: Language not supported
  • source/androidx.media3/media3-exoplayer/Transforms/Metadata.xml: Language not supported
@moljac
Copy link
Contributor Author

moljac commented Jan 15, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@moljac
Copy link
Contributor Author

moljac commented Jan 15, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@moljac
Copy link
Contributor Author

moljac commented Jan 16, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@moljac moljac requested a review from jpobst January 16, 2025 14:27
@jpobst
Copy link
Contributor

jpobst commented Jan 16, 2025

My main concern here is the extensive API breakage these fixes require, particularly:

#### Type Changed: AndroidX.Media3.ExoPlayer.Source.ExternallyLoadedMediaSource.Factory.Factory

Removed interface:
IMediaSourceFactory

Added interface:
IMediaSourceMediaSourceIFactory

Xamarin.AndroidX.Media3.ExoPlayer.dll.diff.md

It appears related to nested interfaces, I wonder if we would be better off opting in to allowing nested interfaces like:

https://github.com/dotnet/android-libraries/pull/956/files#diff-9da24614831c308827a1ae533ffea392c97638c261dd42bd0f5226baa136d16eR2-R6

However this may just create different API breakages.

Given that MAUI community toolkit is already using our existing Media3 packages, perhaps we should ask @ne0rrmatrix to weigh in.

Additionally some other people who appear to already be using these packages from the filed issues:
@ArchangelWTF, @wilsonvargas, @datvm

@moljac
Copy link
Contributor Author

moljac commented Jan 16, 2025

My main concern here is the extensive API breakage these fixes require, particularly:

#### Type Changed: AndroidX.Media3.ExoPlayer.Source.ExternallyLoadedMediaSource.Factory.Factory

Removed interface:
IMediaSourceFactory

Added interface:
IMediaSourceMediaSourceIFactory

The problem was with 2 interfaces MediaSourceFactory and MediaSource.Factory and they inherit from each other. The error was that there was cyclic inheritance/dependency, but it was not, because there are 2 interfaces.
Generator did remove . in 2nd interface and that caused cyclic inheritance and error.

Thus managed side we have IMediaSourceFactory (from java MediaSourceFactory) and IMediaSourceFactory (from java MediaSource.Factory).

Renaming one of them:

    <attr
        path="/api/package[@name='androidx.media3.exoplayer.source']/interface[@name='MediaSource.Factory']"
        name="managedName"
        >
        IMediaSourceIFactory
    </attr>
    <attr
        path="/api/package[@name='androidx.media3.exoplayer.source']/interface[@name='MediaSourceFactory']"
        name="managedName"
        >
        IMediaSourceFactory
    </attr>

Maybe I can try different renaming (to preserve IMediaSourceFactory), but I think there will be some other API breakage.

Xamarin.AndroidX.Media3.ExoPlayer.dll.diff.md

It appears related to nested interfaces, I wonder if we would be better off opting in to allowing nested interfaces like:

https://github.com/dotnet/android-libraries/pull/956/files#diff-9da24614831c308827a1ae533ffea392c97638c261dd42bd0f5226baa136d16eR2-R6

However this may just create different API breakages.

I think so too.

Given that MAUI community toolkit is already using our existing Media3 packages, perhaps we should ask @ne0rrmatrix to weigh in.

Additionally some other people who appear to already be using these packages from the filed issues: @ArchangelWTF, @wilsonvargas, @datvm

@ne0rrmatrix
Copy link
Contributor

If it fixes the issues with java constants and the various interface issues I would like to see it get merged. I am unable to use a few libraries in media 3 due to being unable to call them .

Copy link
Contributor

@jpobst jpobst left a comment

Choose a reason for hiding this comment

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

I guess we don't have much choice other than API breakage. At least these are relatively new packages so they probably don't have many users yet.

@moljac
Copy link
Contributor Author

moljac commented Jan 18, 2025

I guess we don't have much choice other than API breakage. At least these are relatively new packages so they probably don't have many users yet.

Let's do it. Users can hang me.

@moljac moljac merged commit a63f55d into main Jan 18, 2025
6 checks passed
@moljac moljac deleted the mu-20241112-issue-1036-androidx.media3.exoplayer branch January 18, 2025 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HlsMediaSource.Factory is missing? RtspMediaSource.Factory is missing?
3 participants