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 FfmpegAudioRenderer non final or allow custom format selection #10061

Closed
Tolriq opened this issue Mar 13, 2022 · 6 comments
Closed

Make FfmpegAudioRenderer non final or allow custom format selection #10061

Tolriq opened this issue Mar 13, 2022 · 6 comments
Assignees

Comments

@Tolriq
Copy link
Contributor

Tolriq commented Mar 13, 2022

[REQUIRED] Use case description

I have some users with mp3 device decoder that do not properly support gapless and they hear a glitch. Enabling mp3 in the ffmpeg decoder properly fix the issue (I'm already using ffmpeg for alac, flac ...).
The issue is that I'd want that to be optional for mp3 as it will consume more battery on most devices than using the device decoder.

Currently everything is final or package private, so it requires to create a full Renderer wrapper + a RendererCapabilities wrapper to be able to filter the codecs. It would be nice to have a simpler solution.

Proposed solution

Make FfmpegAudioRenderer non final so we can override supportsFormatInternal, or have an optional constructor parameter that accept a callback to do filtering.

Alternatives considered

The current solution is to create a full Renderer + RendererCapabilities wrapper.

@icbaker

This comment was marked as outdated.

@Tolriq
Copy link
Contributor Author

Tolriq commented Mar 14, 2022

I tried that but in Kotlin this does not compile as exposing package private FfmpegAudioDecoder.

The decorator pattern at the Renderer level is what I do and it works, but it's a lot more to delegate. I wondered if there was the place for something more clean to better handle multiple decoders without going down the track selector road.

But I understand the API surface issue, so if there's nothing to be done for that need, I can keep the Renderer delegate.

@icbaker
Copy link
Collaborator

icbaker commented Mar 14, 2022

Ah right, yeah I see my proposal won't work because the FfmpegAudioDecoder isn't public - sorry about that, I'll hide that reply.

Zooming out slightly: It sounds like you've got some files where the platform MediaCodec claims to be able to play them but then incorrectly handles the gapless metadata. And your proposed solution (if i'm understanding correctly) is to use the FfmpegAudioRenderer with DefaultRenderersFactory. extensionRendererMode == EXTENSION_RENDERER_MODE_PREFER so that ffmpeg is used for all playbacks that it supports - and then you want to tweak FfmpegAudioRenderer so that it (incorrectly) claims it doesn't support the subset of files that you know will play correctly with the platform MediaCodec instance.

It sounds a bit strange to solve this by essentially making FfmpegAudioRenderer lie about its capabilities? Given that, it's unlikely we will make changes to FfmpegAudioRenderer to support this use-case.

Other options (beyond just depending on the library locally (which I guess you're already doing if you're using the ffmpeg extension) and maintaining the tiny patch that removes the final keyword):

  • If you can give us some more info about the specific decoder and device you're having problems with, it might be possible to modify the MediaCodec interactions in the library to ignore it and use the AOSP-provided MP3 decoder which is likely also present on the device.
  • You could implement some custom track selection logic that maps tracks to either MediaCodecAudioRenderer or FfmpegAudioRenderer depending on the relevant Format info. This is probably quite involved, but feels quite 'correct' in that you're not having to make FfmpegAudioRenderer lie about what it supports. You will probably need to create a new TrackSelector implementation, you can likely fork DefaultTrackSelector as a starting point.

@Tolriq
Copy link
Contributor Author

Tolriq commented Mar 14, 2022

Actually I use a RenderersFactory to reduce app size.

return arrayOf(
            MpegFilterWrapperRenderer(FfmpegAudioRenderer(eventHandler, audioRendererEventListener), allowMpeg = allowMpeg),
            MediaCodecAudioRenderer(context, MediaCodecSelector.DEFAULT, eventHandler, audioRendererEventListener)
        )

The problem is that it's in order, since I want ffmepg for Flac as lot of devices have issues I put it first, but then if I want to optionally enable mp3 from ffmpeg I need either the renderer or the trackselector thing. (Renderer is easier)

About gapless I think the issue is mostly #8594 but strangely for one user also using ffmpeg does not solve it.

With his media I have no issues on my devices.

He have:

 Android: 11 - 30 [arm64-v8a,armeabi-v7a,armeabi]
Device: samsung - SM-A715W [a71cs - a71]

No idea how to debug but I suppose this require a proper new issue.

@icbaker
Copy link
Collaborator

icbaker commented Mar 15, 2022

I'm going to close this because we're not going to remove the final keyword from FfmpegAudioRenderer or add a specific predicate for format filtering.

@Tolriq
Copy link
Contributor Author

Tolriq commented Mar 15, 2022

@icbaker what about the root gapless issue what do I need to ask from the user? Logs shows no difference between his device and mine during playback, so not sure what to report.

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

3 participants