914479 - AudioToolbox MP3 backend for OSX

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

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

Attachment: MP3 support for OSX

Comment 2

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

Attachment: Build Stuff

Comment 3

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

Attachment: IS THIS THING ON?

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 5

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

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.

Comment 6

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

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok.  I'm not certain how -framework works on Apple's ld so I figured I'd ask.

Comment 8

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

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.

Comment 9

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

(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.

Comment 10

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

(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.

Comment 11

[cajbir (:cajbir)]

(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 12

[Paul Adenot (:padenot)]

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.

Comment 13

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

Attachment: MP3 support for OSX v2

Addressed review comments; switched to MP3FrameParser for duration estimate; added more comments.

Comment 14

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

Comment on attachment 804133 [details] [diff] [review]
MP3 support for OSX v2

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

Ship it.

Comment 15

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

Attachment: MP3 frame parser fix

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.

Comment 16

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

Attachment: MP3 frame parser fix v2

Comment 17

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

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?

Comment 18

[Paul Adenot (:padenot)]

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?

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/21256dcd9709
https://hg.mozilla.org/integration/mozilla-inbound/rev/260b69b01dc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a40aa00819e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f8456871319

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21256dcd9709
https://hg.mozilla.org/mozilla-central/rev/260b69b01dc1
https://hg.mozilla.org/mozilla-central/rev/3a40aa00819e
https://hg.mozilla.org/mozilla-central/rev/0f8456871319

Comment 21

[Florian Bender]

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?

Comment 22

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

(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.

Comment 23

[Morris-moz]

(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).

Comment 24

[Marc Bejarano]

(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.