1431169 - Ambisonic AAC doesn't not decode

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Jean-Yves Avenard [:jya]]

Attachment: ambisonics.mp4

STR: Attempt to play the attached file.

Expected: file play
What happens: you get a decoding error.
in the console you see:
Media resource [...]/ambisonics.mp4 could not be decoded.
ambisonics.mp4
Media resource file:[..]/ambisonics.mp4 could not be decoded, error: Error Code: NS_ERROR_DOM_MEDIA_DECODE_ERR (0x806e0004)
Details: mozilla::MediaResult mozilla::AppleATDecoder::DecodeSample(mozilla::MediaRawData *): Error decoding audio sample: 1852797029 @ 64000
ambisonics.mp4

Comment 1

[Jean-Yves Avenard [:jya]]

I manager to reproduce the same issue with a generated audio file. Anything with 4.0 appears to fail in fact.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1431169 - P1. Add test case.

Review commit: https://reviewboard.mozilla.org/r/213714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213714/

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1431169 - P2. Fix Apple AAC decoder on some files.

It is necessary to provide the AAC's magic cookie to the decoder. We already do so for the Windows and FFmpeg decoder.
It's another case of how did it work before really.

Review commit: https://reviewboard.mozilla.org/r/213716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213716/

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1431169 - P3. Stop playback mochitest on error.

Otherwise we need to wait for the test to timeout as the ended event won't be fired.

Review commit: https://reviewboard.mozilla.org/r/213718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213718/

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8943389 [details]
Bug 1431169 - P1. Add test case.

https://reviewboard.mozilla.org/r/213714/#review219492

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8943390 [details]
Bug 1431169 - P2. Fix Apple AAC decoder on some files.

https://reviewboard.mozilla.org/r/213716/#review219496

::: commit-message-5f78f:3
(Diff revision 1)
> +Bug 1431169 - P2. Fix Apple AAC decoder on some files. r?rillian
> +
> +It is necessary to provide the AAC's magic cookie to the decoder. We already do so for the Windows and FFmpeg decoder.

Would be good to specifically mention multi-channel or ambisonic audio here in the commit message. Easier to find searching later.

::: dom/media/platforms/apple/AppleATDecoder.cpp:617
(Diff revision 1)
> +    status = AudioConverterSetProperty(mConverter,
> +                                       kAudioConverterDecompressionMagicCookie,
> +                                       magicCookie.Length(),
> +                                       magicCookie.Elements());
> +    if (status) {
> +      LOG("Error setting AudioConverter AAC cookie:%lld", int64_t(status));

I guess you're already casting `OSStatus` to `int64_t` elsewhere in the file, so this is fine, but OSStatus is 32 bits, so this seems excessive. Wouldn't `int32_t(status)` or even `int(status)` like you have above work just as well?

::: dom/media/platforms/apple/AppleATDecoder.cpp:618
(Diff revision 1)
> +                                       kAudioConverterDecompressionMagicCookie,
> +                                       magicCookie.Length(),
> +                                       magicCookie.Elements());
> +    if (status) {
> +      LOG("Error setting AudioConverter AAC cookie:%lld", int64_t(status));
> +      mConverter = nullptr;

Do you need to `AudioConverterDispose(mConverter);` here to avoid leaking, like in `ProcessShutdown()`?

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8943391 [details]
Bug 1431169 - P3. Stop playback mochitest on error.

https://reviewboard.mozilla.org/r/213718/#review219502

lgtm. thanks!

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8943389 [details]
Bug 1431169 - P1. Add test case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213714/diff/1-2/

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8943390 [details]
Bug 1431169 - P2. Fix Apple AAC decoder on some files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213716/diff/1-2/

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8943391 [details]
Bug 1431169 - P3. Stop playback mochitest on error.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213718/diff/1-2/

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8943390 [details]
Bug 1431169 - P2. Fix Apple AAC decoder on some files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213716/diff/2-3/

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8943391 [details]
Bug 1431169 - P3. Stop playback mochitest on error.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213718/diff/2-3/

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8943390 [details]
Bug 1431169 - P2. Fix Apple AAC decoder on some files.

https://reviewboard.mozilla.org/r/213716/#review219496

> Do you need to `AudioConverterDispose(mConverter);` here to avoid leaking, like in `ProcessShutdown()`?

we do, thanks for spotting, I had copied the software pattern above

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8943390 [details]
Bug 1431169 - P2. Fix Apple AAC decoder on some files.

https://reviewboard.mozilla.org/r/213716/#review219620

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2856cf89502b
P1. Add test case. r=rillian
https://hg.mozilla.org/integration/autoland/rev/e97d7f1498ad
P2. Fix Apple AAC decoder on some files. r=rillian
https://hg.mozilla.org/integration/autoland/rev/e3c55a7d7642
P3. Stop playback mochitest on error. r=rillian

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2856cf89502b
https://hg.mozilla.org/mozilla-central/rev/e97d7f1498ad
https://hg.mozilla.org/mozilla-central/rev/e3c55a7d7642