Closed Bug 1096769 Opened 10 years ago Closed 10 years ago

Apple AudioToolbox/AAC: Audio doesn't play

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(1 file, 2 obsolete files)

From bug 1073805, 5.1 AAC sample doesn't play.
https://bugzilla.mozilla.org/attachment.cgi?id=8496419

Error in log:
"###!!! ASSERTION: Failed to apply ADTS header: 'Error', file /Users/jyavenard/Work/Mozilla/mozilla-central/dom/media/fmp4/apple/AppleATDecoder.cpp, line 344"

Apple AAC decoder must be able to accept raw AAC frames.
Note this likely involves rewriting AppleATDecoder to call the AudioUnit framework directly instead of using AudioFileStream to set up the converter.

Another idea is to remux the AAC samples into an m4a stream and pass that into AudioFileStream. That's probably even more code though.
Assignee: nobody → jyavenard
Depends on: 1093318
No longer depends on: 1073805
Rewrite mac AAC audio decoder to directly use CoreAudio's AudioConverter without using AudioStream object. Turned out, it's actually much easier to do so that way. It now handles every file I've thrown at it. There is a small regression however where it's unable to find the SBR stream like in http://www-tvline-com.vimg.net/streaming/tvline/MAA101-Medianet.mp4 ; and instead only plays the AAC basic one. Despite my best effort so far, I haven't found a way to extract that information. FFmpeg's demuxer detects it in the demuxer using a MP4's glbl atom, which is a QuickTime extension and that libstagefright doesn't support
Attachment #8523648 - Flags: review?(giles)
Comment on attachment 8523648 [details] [diff] [review]
Rewrite mac audio decoder to support raw AAC

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

That is simpler. Thanks. r=me with comments addressed.

Please open a bug depending on this one for the SBR regression.

::: dom/media/fmp4/apple/AppleATDecoder.cpp
@@ +85,5 @@
> +    if (NS_WARN_IF(status)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    LOG("found 1 available audio stream");
> +  }

It's odd that the kAudioFormatProperty_FormatList method is in a helper, but the kAudioFormatProperty_FormatInfo method is inline here. Please make a method for it too, or just move this into GetInputAudioDescription().

@@ +175,3 @@
>    UInt32 mDataSize;
>    const void* mData;
> +  AudioStreamPacketDescription mPacket[1];

Is mPacket an array here for parallel construction with aData->mBuffers below? Otherwise making it a direct member seems easier.

@@ +218,4 @@
>  {
> +  // Array containing the queued decoded audio frames, about to be output.
> +  nsTArray<AudioDataValue> outputData;
> +  OSStatus rv;

Leave this declaration where it's first assigned, per style.

@@ +260,1 @@
>        LOG("Error decoding audio stream: %d\n", rv);

This is the only place you check for error, so call mCallback->Error() here instead of at the end. You can still break and drain mOutputData, although that deserves a comment. This error flow is harder to understand than it needs to be.

@@ +268,4 @@
>  
> +    if (rv == kNoMoreDataErr) {
> +      LOG("done processing compressed packet");
> +      rv = noErr;

Then you can drop this reset.

@@ +274,2 @@
>    } while (true);
>  

// Flush any decoded data, regardless of whether we hit an error.

@@ +278,2 @@
>      int rate = mOutputFormat.mSampleRate;
> +    Microseconds duration = FramesToUsecs(numFrames, rate).value();

You must check .isValid() before using the value().
Attachment #8523648 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #3)
> Comment on attachment 8523648 [details] [diff] [review]
> Rewrite mac audio decoder to support raw AAC
> Please open a bug depending on this one for the SBR regression.

will do...

> It's odd that the kAudioFormatProperty_FormatList method is in a helper, but
> the kAudioFormatProperty_FormatInfo method is inline here. Please make a
> method for it too, or just move this into GetInputAudioDescription().

it's an evolution on how I was debugging. After a test on 10.6 where it was failing, I thought the issue was that some of the GetProperty call were failing. So the idea was simply to abort at any stage and fall back on the default method.

So in one case of error we can recover, in the other we can't and have to abort.

Yes, will move everything to GetInputAudioDescription() logic is a tad more complicated that way however.


> 
> @@ +175,3 @@
> >    UInt32 mDataSize;
> >    const void* mData;
> > +  AudioStreamPacketDescription mPacket[1];
> 
> Is mPacket an array here for parallel construction with aData->mBuffers
> below? Otherwise making it a direct member seems easier.

I thought it was clearer.

it makes it easier to copy mPacket inside the callback.

> @@ +260,1 @@
> >        LOG("Error decoding audio stream: %d\n", rv);
> 
> This is the only place you check for error, so call mCallback->Error() here
> instead of at the end. You can still break and drain mOutputData, although
> that deserves a comment. This error flow is harder to understand than it
> needs to be.

I thought this made it clearer, it also makes the patch slightly smaller as it's the same logic as in the previous iteration.

Plus to me, draining mOutputData and *then* calling mCallback->Error() seems more intuitive/logical

> 
> @@ +268,4 @@
> >  
> > +    if (rv == kNoMoreDataErr) {
> > +      LOG("done processing compressed packet");
> > +      rv = noErr;
> 
> Then you can drop this reset.

According the Apple's example, testing for the number of packets returned should be enough to test for EOF, no need to check for the error code (actually, Apple's example returns 1 as error code and never bother looking at it again)

> @@ +278,2 @@
> >      int rate = mOutputFormat.mSampleRate;
> > +    Microseconds duration = FramesToUsecs(numFrames, rate).value();
> 
> You must check .isValid() before using the value().

it is impossible for it to ever overflow. numFrames * rate will always be inferior to mSample->duration. 
Also, no other decoder test for this, nor did the original AAC decoder. So to me checking for validity makes it inconsistent.
Carrying r+
Attachment #8523648 - Attachment is obsolete: true
Some notes on why HE-AAC in some stream aren't recognised any longer:
https://developer.apple.com/library/ios/technotes/tn2236/_index.html

Basically, now that we are feeding directly raw aac data, and read the AAC magic cookie only explicit signalling is supported.

We also need implicit signalling support, and only via an ADTS stream can Core Audio's APIs detect it.
Blocks: 1101534
Fix compilation with 10.8 SDK
Attachment #8525312 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f52a81db4ecd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.