Closed Bug 1431169 Opened 2 years ago Closed 2 years ago

Ambisonic AAC doesn't not decode

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(4 files)

Attached video 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
I manager to reproduce the same issue with a generated audio file. Anything with 4.0 appears to fail in fact.
Assignee: nobody → jyavenard
Depends on: 1431221
Comment on attachment 8943389 [details]
Bug 1431169 - P1. Add test case.

https://reviewboard.mozilla.org/r/213714/#review219492
Attachment #8943389 - Flags: review?(giles) → review+
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()`?
Attachment #8943390 - Flags: review?(giles) → review+
Comment on attachment 8943391 [details]
Bug 1431169 - P3. Stop playback mochitest on error.

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

lgtm. thanks!
Attachment #8943391 - Flags: review?(giles) → review+
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 on attachment 8943390 [details]
Bug 1431169 - P2. Fix Apple AAC decoder on some files.

https://reviewboard.mozilla.org/r/213716/#review219620
Pushed by jyavenard@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.