1022501 - MP4 demuxer needs MP3 support

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

MP3 support is missing from the new MP4 demuxer.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add MP3 support to MP4 demuxer

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8436681 [details] [diff] [review]
Add MP3 support to MP4 demuxer

Review of attachment 8436681 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine. I'll try to test this on Windows over the next few days.

::: content/media/fmp4/MP4Reader.cpp
@@ +199,5 @@
>    mInfo.mAudio.mHasAudio = mAudio.mActive = mDemuxer->HasValidAudio();
>    const AudioDecoderConfig& audio = mDemuxer->AudioConfig();
>    // If we have audio, we *only* allow AAC to be decoded.
> +  if (mInfo.mAudio.mHasAudio &&
> +      !mPlatform->SupportsAudioMimeType(audio.mime_type)) {

We still need to be white listing codecs here, rather than just accepting that we should play anything our demuxer/PDM combination reports it's able to play.

::: content/media/fmp4/ffmpeg/FFmpegAACDecoder.h
@@ -19,1 @@
>    FFmpegAACDecoder(MediaTaskQueue* aTaskQueue,

Seems that this should be called FFmpegAudioDecoder now.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add MP3 support to MP4 demuxer

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add MP3 support to MP4 demuxer

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix Windows falsely reporting support for MP4 in MP3

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8456635 [details] [diff] [review]
Fix Windows falsely reporting support for MP4 in MP3

Review of attachment 8456635 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we should do it this way because this means the IsWMFSupportedType will not be reporting what types WMF supports.

I think we should instead check the media.fragmented-mp4.exposed pref at the only callsite where this is relevant; at DecoderTraits.cpp:440

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fix Windows falsely reporting support for MP4 in MP3

Alright then....

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8456634 [details] [diff] [review]
Add MP3 support to MP4 demuxer

Review of attachment 8456634 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/fmp4/MP4Reader.cpp
@@ +192,5 @@
> +MP4Reader::IsSupportedAudioMimeType(const char* aMimeType)
> +{
> +  return (!strcmp(aMimeType, "audio/mpeg") ||
> +          !strcmp(aMimeType, "audio/mp4a-latm")) &&
> +         mPlatform->SupportsAudioMimeType(aMimeType);

Do we need to check the MIME type here? SupportsAudioMimeType does that.

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #8)
> Do we need to check the MIME type here? SupportsAudioMimeType does that.

See comment #2.

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Add MP3 support to MP4 demuxer

Bug 1022501- Add MP3 support to MP4 demuxer, MP3 support to Apple CoreAudio decoder and FFMpeg > 54

Comment 11

[Jean-Yves Avenard [:jya]]

https://tbpl.mozilla.org/?tree=Try&rev=acf2a451f0d8

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: Add MP3 support to MP4 demuxer

Bug 1022501- Add MP3 support to MP4 demuxer, MP3 support to Apple CoreAudio decoder and FFMpeg > 54. Fix Gonk build

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: Add MP3 support to MP4 demuxer

Do not make SupportsAudioMimeType pure virtual. Have a default implementation that only accepts AAC

Comment 14

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8472153 [details] [diff] [review]
Add MP3 support to MP4 demuxer

Review of attachment 8472153 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: content/media/fmp4/apple/AppleATDecoder.cpp
@@ +46,5 @@
> +
> +  if (!strcmp(aConfig.mime_type, "audio/mpeg")) {
> +    mFileType = kAudioFileMP3Type;
> +  } else {
> +    mFileType = kAudioFileAAC_ADTSType;

check the mimetype after the |else| as well, and return an error from Init() if we don't recognise it.

Comment 15

[Jean-Yves Avenard [:jya]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #14)
> Comment on attachment 8472153 [details] [diff] [review]
> Add MP3 support to MP4 demuxer
> 
> Review of attachment 8472153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: content/media/fmp4/apple/AppleATDecoder.cpp
> @@ +46,5 @@
> > +
> > +  if (!strcmp(aConfig.mime_type, "audio/mpeg")) {
> > +    mFileType = kAudioFileMP3Type;
> > +  } else {
> > +    mFileType = kAudioFileAAC_ADTSType;
> 
> check the mimetype after the |else| as well, and return an error from Init()
> if we don't recognise it.

We can only be called with a mimetype of mp3 or AAC due to AppleDecoderModule::SupportsAudioMimeType implementation. Any other mimetype would have caused an error before AppleATDecoder constructor is called

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: Add MP3 support to MP4 demuxer

Carrying r+

Comment 17

[Jean-Yves Avenard [:jya]]

https://tbpl.mozilla.org/?tree=Try&rev=5da2874a28ec

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8dc9ac0228
https://hg.mozilla.org/integration/mozilla-inbound/rev/9329fb07c373

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows mochitest-1 failures. Looks like the Try push didn't include Anthony's patch?
https://hg.mozilla.org/integration/mozilla-inbound/rev/192adef87e6a

https://tbpl.mozilla.org/php/getParsedLog.php?id=46043792&tree=Mozilla-Inbound

Comment 20

[Jean-Yves Avenard [:jya]]

Attachment: Fix Windows falsely reporting support for MP3 in fragmented MP4

Fix mochitest

Comment 21

[Jean-Yves Avenard [:jya]]

https://tbpl.mozilla.org/?tree=Try&rev=32e43af6a583

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51b043a9cc3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/945fe09a569a

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51b043a9cc3f
https://hg.mozilla.org/mozilla-central/rev/945fe09a569a