1314794 - Only use AndroidDecoderModule for particular codecs

Status: NEW

(Core :: Audio/Video: Playback, defect, P3)

Description

[Ralph Giles (:rillian)]

We prefer to use generic software decoders where the platform decoder doesn't have a specific advantage. This bug is a follow up to implement Jean-Yves review comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1266792#c34

> I think it would be preferable to only call FindDecodercodecInfoForMimeType
> for type we do want: e.g: h264, aac, mp3, vp9, vp8

Comment 1

[Ralph Giles (:rillian)]

Probably we need an equivalent of VPXDecoder::IsVPX(aMimeType) for the other codecs we want to support.

Comment 2

[Ralph Giles (:rillian)]

Attachment: Bug 1314794 - Add MP3Decoder::IsMP3() call.

Similar to VPXDecoder::IsVP9() provide a utility call for
checking if a mime type represents an mp3 track.

Review commit: https://reviewboard.mozilla.org/r/90204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90204/

Comment 3

[Ralph Giles (:rillian)]

Attachment: Bug 1314794 - Add ADTSDecoder::IsAAC() call.

Similar to VPXDecoder::IsVP9() provide a utility call for
checking if a mime type represents an aac track.

Review commit: https://reviewboard.mozilla.org/r/90206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90206/

Comment 4

[Ralph Giles (:rillian)]

Attachment: Bug 1314794 - Only use AndroidDecoderModule for particular codecs.

Check the mime type against a white list where we prefer the android
platform decoders, using the new ::IsFoo() static methods.

Review commit: https://reviewboard.mozilla.org/r/90208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90208/

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8806970 [details]
Bug 1314794 - Add ADTSDecoder::IsAAC() call.

https://reviewboard.mozilla.org/r/90206/#review89922

::: dom/media/ADTSDecoder.cpp:46
(Diff revision 1)
> +ADTSDecoder::IsAAC(const nsACString& aType)
> +{
> +  return aType.EqualsASCII("audio/aac")
> +    || aType.EqualsASCII("audio/aacp")
> +    || aType.EqualsASCII("audio/x-aac");
> +}

Do I need to worry about "audio/mp4a-latm" here?

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8806969 [details]
Bug 1314794 - Add MP3Decoder::IsMP3() call.

https://reviewboard.mozilla.org/r/90204/#review90562

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8806969 [details]
Bug 1314794 - Add MP3Decoder::IsMP3() call.

https://reviewboard.mozilla.org/r/90204/#review90564

OOPS my bad... all those are the mimetype for the container, not the codec.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8806970 [details]
Bug 1314794 - Add ADTSDecoder::IsAAC() call.

https://reviewboard.mozilla.org/r/90206/#review90566

those methods cant belong in the MediaDecoders really.
Gerald has started to add new classes to help distinguish the mimetypes (container vs codec). Maybe it could belong there as statics

::: dom/media/ADTSDecoder.cpp:46
(Diff revision 1)
> +ADTSDecoder::IsAAC(const nsACString& aType)
> +{
> +  return aType.EqualsASCII("audio/aac")
> +    || aType.EqualsASCII("audio/aacp")
> +    || aType.EqualsASCII("audio/x-aac");
> +}

audio/mp4a-latm is the mimetype for the codec.
audio/mpeg or audio/mp3 is the mimetype for the container.

the mimetype used in the MediaDecoder have nothing to do with the mimetypes used by the MediaDataDecoder.

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8806971 [details]
Bug 1314794 - Only use AndroidDecoderModule for particular codecs.

https://reviewboard.mozilla.org/r/90208/#review90568

::: dom/media/platforms/android/AndroidDecoderModule.cpp:164
(Diff revision 1)
> -  if (aMimeType.EqualsLiteral("audio/opus")) {
> -    LOG("Rejecting audio of type %s", aMimeType.Data());
> -    return false;
> -  }
> -
> +  // should use a platform-independent module.
> +  if (VPXDecoder::IsVP8(aMimeType) ||
> +      VPXDecoder::IsVP9(aMimeType) ||
> +      MP4Decoder::IsH264(aMimeType) ||
> +      MP3Decoder::IsMP3(aMimeType) ||
> +      ADTSDecoder::IsAAC(aMimeType)) {

different mimetype, different decoder (MediaDataDecoder vs MediaDecoder)

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8806970 [details]
Bug 1314794 - Add ADTSDecoder::IsAAC() call.

Bug 1314858 added MP4Decoder::IsAAC which we can use instead.

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8806971 [details]
Bug 1314794 - Only use AndroidDecoderModule for particular codecs.

https://reviewboard.mozilla.org/r/90208/#review91896

I was only waiting for the new version, but it's making noise in my pending list of review... so for now...

Comment 12

[Ralph Giles (:rillian)]

Yeah, sorry I haven't gotten back to it yet.

Comment 13

[Ralph Giles (:rillian)]

I never followed up on this and won't be able to complete it. Free for anyone else to take over.