Closed Bug 1052383 Opened 5 years ago Closed 5 years ago

Mac doesn't play HE-AAC

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(2 files, 4 obsolete files)

Mac doesn't detect SRS/PS from HE-AAC stream, playing only the AAC (AAC-LC) stream
Depends on: 1047180
Attached patch Properly detect HE-AAC stream (obsolete) — Splinter Review
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.
Attachment #8471524 - Flags: review?(giles)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Attached patch Properly detect HE-AAC stream (obsolete) — Splinter Review
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.
Attachment #8472064 - Flags: review?(giles)
Attachment #8471524 - Attachment is obsolete: true
Attachment #8471524 - Flags: review?(giles)
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.
Attachment #8472064 - Flags: review?(giles) → review-
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
Attachment #8472796 - Flags: review?(giles)
Attached patch Properly detect HE-AAC stream (obsolete) — Splinter Review
Use helper to retrieve best playable format
Attachment #8472800 - Flags: review?(giles)
Attachment #8472064 - Attachment is obsolete: true
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
Attached patch Properly detect HE-AAC stream (obsolete) — Splinter Review
Use helper to retrieve best playable format. Fix spelling in one of the comment
Attachment #8472802 - Flags: review?(giles)
Attachment #8472800 - Attachment is obsolete: true
Attachment #8472800 - Flags: review?(giles)
Attachment #8472796 - Flags: review?(giles) → review+
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.'
Attachment #8472802 - Flags: review?(giles) → review+
(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+
Attachment #8472802 - Attachment is obsolete: true
Keywords: checkin-needed
(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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.