Closed Bug 1022501 Opened 10 years ago Closed 10 years ago

MP4 demuxer needs MP3 support

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: ajones, Assigned: jya)

References

Details

Attachments

(2 files, 8 obsolete files)

50.46 KB, patch
Details | Diff | Splinter Review
1.22 KB, patch
Details | Diff | Splinter Review
MP3 support is missing from the new MP4 demuxer.
Attached patch Add MP3 support to MP4 demuxer (obsolete) — Splinter Review
Attachment #8436681 - Flags: feedback?(cpearce)
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.
Attachment #8436681 - Flags: feedback?(cpearce) → feedback+
Attachment #8436681 - Attachment is obsolete: true
Attachment #8455132 - Attachment is obsolete: true
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
Attachment #8456635 - Flags: review?(cpearce) → review-
Alright then....
Attachment #8457767 - Flags: review?(cpearce)
Attachment #8456635 - Attachment is obsolete: true
Attachment #8457767 - Flags: review?(cpearce) → review+
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.
Attachment #8456634 - Flags: review?(edwin) → review+
(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.
Assignee: ajones → jyavenard
Depends on: 1047180
Attached patch Add MP3 support to MP4 demuxer (obsolete) — Splinter Review
Bug 1022501- Add MP3 support to MP4 demuxer, MP3 support to Apple CoreAudio decoder and FFMpeg > 54
Attachment #8456634 - Attachment is obsolete: true
Attachment #8470797 - Flags: review?(edwin)
Attached patch Add MP3 support to MP4 demuxer (obsolete) — Splinter Review
Bug 1022501- Add MP3 support to MP4 demuxer, MP3 support to Apple CoreAudio decoder and FFMpeg > 54. Fix Gonk build
Attachment #8471312 - Flags: review?(edwin)
Attachment #8470797 - Attachment is obsolete: true
Attachment #8470797 - Flags: review?(edwin)
Attached patch Add MP3 support to MP4 demuxer (obsolete) — Splinter Review
Do not make SupportsAudioMimeType pure virtual. Have a default implementation that only accepts AAC
Attachment #8472153 - Flags: review?(edwin)
Attachment #8471312 - Attachment is obsolete: true
Attachment #8471312 - Flags: review?(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.
Attachment #8472153 - Flags: review?(edwin) → review+
(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
Carrying r+
Attachment #8472153 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8457767 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/51b043a9cc3f
https://hg.mozilla.org/mozilla-central/rev/945fe09a569a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.