Closed Bug 914479 Opened 11 years ago Closed 11 years ago

AudioToolbox MP3 backend for OSX

Categories

(Core :: Audio/Video, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
relnote-firefox --- 26+

People

(Reporter: eflores, Assigned: eflores)

References

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 2 obsolete files)

      No description provided.
Attached patch MP3 support for OSX (obsolete) — Splinter Review
Attachment #802038 - Flags: review?(cpearce)
Comment on attachment 802039 [details] [diff] [review]
Build Stuff

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

::: content/media/apple/Makefile.in
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +LDFLAGS += \
> +    -framework AudioToolbox \
> +    $(NULL)

Are you sure you don't need this when linking libxul too?
Attachment #802039 - Flags: review?(khuey) → review+
Comment on attachment 802038 [details] [diff] [review]
MP3 support for OSX

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

How much more work would it be to make this support audio/aac, audio/aac-adts and audio/mp4? If we can get that cheaply, now, without having to wait for a video backend, then I think we should do it. I'll refrain from r+ing this patch until we know the answer to this, otherwise I'd be willing to r+ it.

I think padenot should also look at this, particularly the seeking code.

::: content/media/DecoderTraits.cpp
@@ +579,5 @@
>      decoderReader = new WMFReader(aDecoder);
>    } else
>  #endif
> +#ifdef MOZ_APPLEMEDIA
> +  if (IsAppleSupportedType(aType)) {

I'd prefer IsAppleMediaSupportedType()

::: content/media/apple/AppleDecoder.h
@@ +14,5 @@
> +public:
> +  AppleDecoder();
> +
> +  MediaDecoder* Clone();
> +  MediaDecoderStateMachine* CreateStateMachine();

Add virtual and MOZ_OVERRIDE keywords to all functions overriding, including those in other classes in this patch please.

::: content/media/apple/AppleMP3Reader.cpp
@@ +198,5 @@
> +  MediaResource *resource = mDecoder->GetResource();
> +
> +  char bytes[AUDIO_READ_BYTES];
> +  uint32_t numBytes;
> +  NS_ENSURE_SUCCESS(resource->Read(bytes, AUDIO_READ_BYTES, &numBytes),

MediaResource::Read() returns early if it doesn't have enough data to fulfil the request. So how about you call this in a loop until totalBytesRead == AUDIO_READ_BYTES, then you won't be calling into AudioFileStreamParseBytes() with a small amount of data in a tight loop if we're running low on downloaded data?

Also, you need to mAudioQueue.Finish() before returning here, until we make DecodeLoop do that for us.

@@ +278,5 @@
> +{
> +  MOZ_ASSERT(mDecoder->OnDecodeThread(), "Should be on decode thread");
> +
> +  *aTags = nullptr;
> +  aInfo->mHasAudio = true;

You should only set mHasAudio=true if the audio decoder was successfully initialized.

@@ +387,5 @@
> +  outputFormat.mBitsPerChannel = 32;
> +  outputFormat.mFormatFlags =
> +    kLinearPCMFormatFlagIsFloat |
> +    0;
> +#elif defined(MOZ_SAMPLE_TYPE_S16)

You don't need this path, MOZ_SAMPLE_TYPE_S16 is only used on android:
http://mxr.mozilla.org/mozilla-central/source/configure.in#5308

You could #error here instead (or at the top of the file) if you're feeling paranoid.

@@ +429,5 @@
> +{
> +  MOZ_ASSERT(mDecoder->OnDecodeThread(), "Should be on decode thread");
> +  NS_ASSERTION(aStartTime < aEndTime, "Seeking should happen over a positive range");
> +
> +  SInt64 packet = aTime * mAudioRate / 1000000 / mAudioFramesPerCompressedPacket;

Use USECS_PER_S instead of 1000000.

I'm not sure if this will be correct, because if the frames are variable sized, or variable bitrate, then this estimate could be off. But I don't think we could do much better, so I'm OK with running with this until it proves to be a problem.

::: content/media/apple/AppleMP3Reader.h
@@ +10,5 @@
> +#include <AudioToolbox/AudioToolbox.h>
> +
> +namespace mozilla {
> +
> +class AppleMP3Reader : public MediaDecoderReader

If we're going to support AAC using this same class, this should be renamed AppleAudioReader.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> Comment on attachment 802039 [details] [diff] [review]
> Build Stuff
> 
> Review of attachment 802039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/apple/Makefile.in
> @@ +3,5 @@
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +LDFLAGS += \
> > +    -framework AudioToolbox \
> > +    $(NULL)
> 
> Are you sure you don't need this when linking libxul too?

I hadn't considered that, but it seems to build fine locally and on try.
Ok.  I'm not certain how -framework works on Apple's ld so I figured I'd ask.
Comment on attachment 802038 [details] [diff] [review]
MP3 support for OSX

(In reply to Chris Pearce (:cpearce) from comment #5)
> How much more work would it be to make this support audio/aac,
> audio/aac-adts and audio/mp4? If we can get that cheaply, now, without
> having to wait for a video backend, then I think we should do it. I'll
> refrain from r+ing this patch until we know the answer to this, otherwise
> I'd be willing to r+ it.

Looks like it should be trivial going by the API docs.

> I think padenot should also look at this, particularly the seeking code.

Adding f? padenot.
Attachment #802038 - Flags: feedback?(paul)
(In reply to Edwin Flores [:eflores] [:edwin] from comment #8)
> Comment on attachment 802038 [details] [diff] [review]
> MP3 support for OSX
> 
> (In reply to Chris Pearce (:cpearce) from comment #5)
> > How much more work would it be to make this support audio/aac,
> > audio/aac-adts and audio/mp4? If we can get that cheaply, now, without
> > having to wait for a video backend, then I think we should do it. I'll
> > refrain from r+ing this patch until we know the answer to this, otherwise
> > I'd be willing to r+ it.
> 
> Looks like it should be trivial going by the API docs.

Do it. Please add AAC support to this patch. We don't want to wait for our own MP4 demuxer to land for AAC support on mac if we don't have to.
(In reply to Chris Pearce (:cpearce) from comment #5)
> @@ +429,5 @@
> > +{
> > +  MOZ_ASSERT(mDecoder->OnDecodeThread(), "Should be on decode thread");
> > +  NS_ASSERTION(aStartTime < aEndTime, "Seeking should happen over a positive range");
> > +
> > +  SInt64 packet = aTime * mAudioRate / 1000000 / mAudioFramesPerCompressedPacket;
> 
> Use USECS_PER_S instead of 1000000.
> 
> I'm not sure if this will be correct, because if the frames are variable
> sized, or variable bitrate, then this estimate could be off. But I don't
> think we could do much better, so I'm OK with running with this until it
> proves to be a problem.

This isn't an estimate.

I should have named |mAudioRate| better. It is the sample rate, not the bit rate. Further, the number of samples per frame (= frames per packet, in Apple speak) is fixed at 1152 in MP3.

Not sure whether this is also the case for MP4.
(In reply to Chris Pearce (:cpearce) from comment #9)
> 
> Do it. Please add AAC support to this patch. We don't want to wait for our
> own MP4 demuxer to land for AAC support on mac if we don't have to.

When you've added AAC please update https://developer.mozilla.org/en-US/docs/HTML/Supported_media_formats.
Comment on attachment 802038 [details] [diff] [review]
MP3 support for OSX

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

f+ on the seeking part.

::: content/media/apple/AppleMP3Reader.cpp
@@ +429,5 @@
> +{
> +  MOZ_ASSERT(mDecoder->OnDecodeThread(), "Should be on decode thread");
> +  NS_ASSERTION(aStartTime < aEndTime, "Seeking should happen over a positive range");
> +
> +  SInt64 packet = aTime * mAudioRate / 1000000 / mAudioFramesPerCompressedPacket;

Yes, the number of samples per packet is fixed for MPEG Layer II and III (1152 frames per packet), so this seems correct. iirc, it's something like 1024 for AAC, so we should not have to rewrite this when we support AAC, but don't quote me on this.
Attachment #802038 - Flags: feedback?(paul) → feedback+
Addressed review comments; switched to MP3FrameParser for duration estimate; added more comments.
Attachment #802038 - Attachment is obsolete: true
Attachment #802038 - Flags: review?(cpearce)
Attachment #804133 - Flags: review?(cpearce)
Comment on attachment 804133 [details] [diff] [review]
MP3 support for OSX v2

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

Ship it.
Attachment #804133 - Flags: review?(cpearce) → review+
Attached patch MP3 frame parser fix (obsolete) — Splinter Review
The duration estimation in MP3FrameParser goes a bit wonky while seeking, which is failing tests. This estimation should be just as good, and a bit more robust.
Attachment #804250 - Flags: review?(paul)
Attachment #804250 - Attachment is obsolete: true
Attachment #804250 - Flags: review?(paul)
Attachment #804261 - Flags: review?(paul)
Comment on attachment 802041 [details] [diff] [review]
IS THIS THING ON?

Green locally. Try run:
https://tbpl.mozilla.org/?tree=Try&rev=448a265135ea

r? contingent on green try?
Attachment #802041 - Flags: review?(cpearce)
Attachment #802041 - Flags: review?(cpearce) → review+
Target Milestone: --- → mozilla26
Comment on attachment 804261 [details] [diff] [review]
MP3 frame parser fix v2

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

::: content/media/MP3FrameParser.cpp
@@ +293,5 @@
>  // skipped bytes to be read, just in case, before we give up and assume
>  // we're not parsing an MP3 stream.
>  static const uint32_t MAX_SKIPPED_BYTES = 200 * 1024;
>  
> +static const uint32_t SAMPLES_PER_FRAME = 1152;

Can you add a comment on this?
Attachment #804261 - Flags: review?(paul) → review+
Assignee: nobody → edwin
Exciting! Soundcloud finally works without Flash!

(In reply to Chris Pearce (:cpearce) from comment #9)
> Do it. Please add AAC support to this patch. We don't want to wait for our
> own MP4 demuxer to land for AAC support on mac if we don't have to.

AFAICS in the patches, this did not happen. Oversight? Any follow-up bug filed?
(In reply to Florian Bender from comment #21)
> Exciting! Soundcloud finally works without Flash!
> 
> (In reply to Chris Pearce (:cpearce) from comment #9)
> > Do it. Please add AAC support to this patch. We don't want to wait for our
> > own MP4 demuxer to land for AAC support on mac if we don't have to.
> 
> AFAICS in the patches, this did not happen. Oversight? Any follow-up bug
> filed?

The AudioToolbox audio parser doesn't seem to support demuxing MP4 for AAC in MP4. It supports AAC-ADTS, but I'm not too sure that's something we want on desktop.

I can start looking at AAC when we have our unified MP4 demuxer, which Chris is working on at the moment.
(In reply to Paul Adenot (:padenot) (On PTO, does not read bugmail) from comment #12)
> 
> Yes, the number of samples per packet is fixed for MPEG Layer II and III
> (1152 frames per packet), so this seems correct. iirc, it's something like
> 1024 for AAC, so we should not have to rewrite this when we support AAC, but
> don't quote me on this.

Sorry for nitpicking. For AAC it can be either 1024 or 960 per the standard. 1024 is more common and many decoding libraries such as FFmpeg have cut corners by not implementing the 960 variant (https://ffmpeg.org/trac/ffmpeg/ticket/1407). However, this is causing trouble with digital audio broadcasting streams which often use 960, leading to a distorted sound (they sound too slow).
(In reply to Morris-moz from comment #23) 
> Sorry for nitpicking. For AAC it can be either 1024 or 960 per the standard.

morris: please file a new issue about this and let us know about it here.  this bug is just to track the landing of the original patch which has already happened.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: