Mac doesn't detect SRS/PS from HE-AAC stream, playing only the AAC (AAC-LC) stream

Properly detect HE-AAC stream. Use the first playable format. The first playable format is the highest quality (or best) format the platform is capable of playing back as determined at run-time.

Properly detect HE-AAC stream. Use the first playable format. The first playable format is the highest quality (or best) format the platform is capable of playing back as determined at run-time.

Comment on attachment 8472064 [details] [diff] [review] Properly detect HE-AAC stream Review of attachment 8472064 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the style fixes. Please split them into a separate patch. For the functional patch, please add a brief paragraph to the commit message explaining the motivation for the change so readers don't have to go digging through bugzilla. The approach looks fine, I'd just like the code re-arranged a bit. The new additions make SetupDecoder() hard to read, with too many conditionals and early returns. If feels like all this should be a helper function. Really, it's too bad we can't use something like CMAudioFormatDescriptionGetRichestDecodableFormat() but that's 10.7 and later. Another thing which would help is to put the old behaviour first, then call a function to update inputFormat with the HE-AAC handling. That way you can just give up and it will use baseline decoding if you hit an error there. ::: content/media/fmp4/apple/AppleATDecoder.cpp @@ +312,5 @@ > + kAudioFileStreamProperty_FormatList, > + &propertySize, > + NULL); > + if (rv == noErr) { > + // allocate memory for the format list items Sentence formatting (initial capital and final period) on comments to match local style, please. @@ +314,5 @@ > + NULL); > + if (rv == noErr) { > + // allocate memory for the format list items > + formatListPtr = static_cast<AudioFormatListItem*>(malloc(propertySize)); > + if (!formatListPtr) { Gecko's malloc is infallible; you don't need to check for nullptr here. It is confusing threading the free() calls through the error handling. Make it an nsAutoPtr instead. Or if we know the size of AudioFormatListItem, can you make it an nsTArray<> and avoid the malloc call? @@ +320,5 @@ > + mCallback->Error(); > + return; > + } > + > + // get the list of Audio Format List Item's I'm not sure this comment is worth trying to make grammatical. Just remove it. @@ +325,5 @@ > + rv = AudioFileStreamGetProperty(mStream, > + kAudioFileStreamProperty_FormatList, > + &propertySize, > + formatListPtr); > + if (rv == noErr) { If the first GetProperty failed, the second will return garbage. Better to just return and save an indentation level. @@ +330,5 @@ > + UInt32 itemIndex; > + UInt32 indexSize = sizeof(itemIndex); > + > + // get the index number of the first playable format -- this index number will be for > + // the highest quality layer the platform is capable of playing Please wrap comments to 72 characters. @@ +336,5 @@ > + propertySize, > + formatListPtr, > + &indexSize, > + &itemIndex); > + if (rv != noErr) { You can just do 'if (rv)' here like in the rest of the file.

I also want to see a test, either as a third patch here, or in a separate bug.

(In reply to Ralph Giles (:rillian) from comment #5) > I also want to see a test, either as a third patch here, or in a separate > bug. Is there even a test for mac simple AAC playback? or the whole mac media decoder?

Separate style fixes

Use helper to retrieve best playable format

Implemented a helper like you suggested... it is indeed much more readable that way. Using CMAudioFormatDescriptionGetRichestDecodableFormat in 10.7, wouldn't have simplified much more, would still have had to retrieve the list of all the formats available, which is the core of the function

Use helper to retrieve best playable format. Fix spelling in one of the comment

Comment on attachment 8472802 [details] [diff] [review] Properly detect HE-AAC stream Review of attachment 8472802 [details] [diff] [review]: ----------------------------------------------------------------- Much better, thanks. r=me with nits addressed. ::: content/media/fmp4/apple/AppleUtils.cpp @@ +80,5 @@ > CFDictionarySetValue(dict, keyRef, value ? kCFBooleanTrue : kCFBooleanFalse); > } > > +nsresult > +AppleUtils::GetRichestDecodableFormat(AudioFileStreamID aAudioFileStream, anAudioFileStream @@ +87,5 @@ > + // Fill in the default format description from the stream. > + nsresult rv = GetProperty(aAudioFileStream, > + kAudioFileStreamProperty_DataFormat, > + &aFormat); > + NS_ENSURE_SUCCESS(rv, rv); We prefer not to use NS_ENSURE_SUCCESS() because it hides a return. Not that you'd know that from the comments in nsDebug.h, or Chris' use in MP4Reader... Please use something with an explicit return like: if (NS_WARN_IF(NS_FAILED(rv)) { return rv; } @@ +95,5 @@ > + kAudioFileStreamProperty_FormatList, > + &propertySize, > + NULL); > + if (status) { > + return NS_OK; Please either NS_WARNING here about the fallback, or document with a comment that we failed, but are falling back to the previously set default, to clarify why you're returning NS_OK. @@ +98,5 @@ > + if (status) { > + return NS_OK; > + } > + > + uint32_t sizeList = propertySize / sizeof(AudioFormatListItem); I would MOZ_ASSERT(propertySize % sizeof(AudioFormatListItem) == 0); here. @@ +108,5 @@ > + NS_ENSURE_SUCCESS(rv, NS_OK); > + > + // Get the index number of the first playable format. > + // This index number will be for the highest quality layer the platform > + // is capable of playing Period at the end of the sentence. ::: content/media/fmp4/apple/AppleUtils.h @@ +32,5 @@ > bool value); > + > + // Helper to retrieve the best audio format available in the given > + // audio stream. > + // The basic format will be returned by default should an error occurred. 'should an error occur.'

(In reply to Ralph Giles (:rillian) from comment #11) > anAudioFileStream No.

(In reply to Ralph Giles (:rillian) from comment #11) > > +nsresult > > +AppleUtils::GetRichestDecodableFormat(AudioFileStreamID aAudioFileStream, > > anAudioFileStream ? where the n would come from? I thought a meant "argument"... > > + nsresult rv = GetProperty(aAudioFileStream, > > + kAudioFileStreamProperty_DataFormat, > > + &aFormat); > > + NS_ENSURE_SUCCESS(rv, rv); > > We prefer not to use NS_ENSURE_SUCCESS() because it hides a return. Not that > you'd know that from the comments in nsDebug.h, or Chris' use in MP4Reader... that seems clear enough to me, NS_ENSURE_SUCCESS returns the 2nd argument, should the first not be equal to NS_OK It's also how it's used thorough the current media code... it's hard to know what to do where there doesn't seem to be a universal consensus on how to use macros. > Please either NS_WARNING here about the fallback, or document with a comment > that we failed, but are falling back to the previously set default, to > clarify why you're returning NS_OK. I will go the comment way, returning something else that NS_OK only makes it more complicated to test the result later on

NS_ENSURE_SUCCESS() is, in my opinion, much more convenient, consistent, and requires less parsing than the more verbose alternatives.

Carrying r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/77025cd1557d https://hg.mozilla.org/integration/mozilla-inbound/rev/159917b54e19

(In reply to Jean-Yves Avenard [:jya] from comment #13) > > anAudioFileStream > > ? where the n would come from? We use 'a' as a function argument prefix because it's mnemonic of the English definite article, which is 'an' before a vowel. I don't feel strongly about it. > it's hard to know what to do where there doesn't seem to be a universal > consensus on how to use macros. Yes it is. I do prefer not to have NS_ASSURE in code I'm reviewing, at least. > > Please either NS_WARNING here about the fallback, or document with a comment > > that we failed, but are falling back to the previously set default, to > > clarify why you're returning NS_OK. > > I will go the comment way, returning something else that NS_OK only makes it > more complicated to test the result later on NS_WARNING doesn't return anything. See how that's confusing? :)

https://hg.mozilla.org/mozilla-central/rev/77025cd1557d https://hg.mozilla.org/mozilla-central/rev/159917b54e19