Closed
Bug 1022501
Opened 10 years ago
Closed 10 years ago
MP4 demuxer needs MP3 support
Categories
(Core :: Audio/Video, defect)
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.
Reporter | ||
Comment 1•10 years ago
|
||
Attachment #8436681 -
Flags: feedback?(cpearce)
Comment 2•10 years ago
|
||
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+
Reporter | ||
Comment 3•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #8436681 -
Attachment is obsolete: true
Reporter | ||
Comment 4•10 years ago
|
||
Attachment #8456634 -
Flags: review?(edwin)
Reporter | ||
Updated•10 years ago
|
Attachment #8455132 -
Attachment is obsolete: true
Reporter | ||
Comment 5•10 years ago
|
||
Attachment #8456635 -
Flags: review?(cpearce)
Comment 6•10 years ago
|
||
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-
Reporter | ||
Updated•10 years ago
|
Attachment #8456635 -
Attachment is obsolete: true
Updated•10 years ago
|
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+
Reporter | ||
Comment 9•10 years ago
|
||
(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 | ||
Comment 10•10 years ago
|
||
Bug 1022501- Add MP3 support to MP4 demuxer, MP3 support to Apple CoreAudio decoder and FFMpeg > 54
Assignee | ||
Updated•10 years ago
|
Attachment #8456634 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8470797 -
Flags: review?(edwin)
Assignee | ||
Comment 11•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=acf2a451f0d8
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8470797 -
Attachment is obsolete: true
Attachment #8470797 -
Flags: review?(edwin)
Assignee | ||
Comment 13•10 years ago
|
||
Do not make SupportsAudioMimeType pure virtual. Have a default implementation that only accepts AAC
Attachment #8472153 -
Flags: review?(edwin)
Assignee | ||
Updated•10 years ago
|
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+
Assignee | ||
Comment 15•10 years ago
|
||
(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
Assignee | ||
Comment 16•10 years ago
|
||
Carrying r+
Assignee | ||
Updated•10 years ago
|
Attachment #8472153 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5da2874a28ec
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8dc9ac0228 https://hg.mozilla.org/integration/mozilla-inbound/rev/9329fb07c373
Keywords: checkin-needed
Comment 19•10 years ago
|
||
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
Assignee | ||
Comment 20•10 years ago
|
||
Fix mochitest
Assignee | ||
Updated•10 years ago
|
Attachment #8457767 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=32e43af6a583
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 22•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/51b043a9cc3f https://hg.mozilla.org/integration/mozilla-inbound/rev/945fe09a569a
Keywords: checkin-needed
Comment 23•10 years ago
|
||
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.
Description
•