1747760 - Improve audio codec specific data handling in AudioInfo

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, task, P3)

Description

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

We can improve our handling of codec specific data by imposing more typing upon it and by removing the duplication in the members we use to store that data.

Having extra typing rather than just a binary blob will have various benefits:

• Easier to find where data is used by searching for types, binary blobs are hard to track.
• Better self documenting types -- hard to know what data is being stored in a binary blob compared to a explicit data structure.
• Better type checking.
• And more.

Getting this bug up so I can file some WIP against it, as I may need to continue the work on another machine.

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1747760 - P1: Add audio codec specific info variant to help give more type info for codec specific data. r?padenot,kinetik

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1747760 - P2: Add IPC code for new codec types. r?padenot,kinetik

Depends on D134727

Comment 3

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1747760 - P3: Rework audio codec specific data to use new variant with a generic blob. r?padenot,kinetik

This switches existing code to use the new variant style codec specific
information for audio, but retains the binary blob style apporach everywhere.

There should be no functional changes following this refactor. Internally all
codec specific data is still stored in blobs. This allows a baseline from which
we can modify specific codecs to use more constrained versions of the variant.

Depends on D134728

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1747760 - P4: Handle mp3 codec specific data using mp3 codec specific variant. r?kinetik,padenot

This switches the mp3 codec specific handling from using a binary blob to
instead use a typed structure.

Depends on D134729

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1747760 - P5: Handle flac codec specific data using flac codec specific variant. r?kinetik,padenot

This also populates flac codec specific data for flac in mp4, which appears to
have been overlooked.

Depends on D134730

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1747760 - P6: Use Wave specific codec data for Wave audio. r?padenot,kinetik

We don't store Wave specific info, but this means that we have a struct in place
if we ever want to add such data + gives us more typing info for codec specific
data.

Depends on D134731

Comment 7

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1747760 - P7: Handle aac codec specific data using aac codec specific variant. r?kinetik,padenot

This should be the last change needed for a number of our platform specific
decoders to be switched entirely over to the new variant types. As such, we can
move them from the lenient ForceGetAudioCodecSpecificBlob and to
GetAudioCodecSpecificBlob to enforce handling of codecs that don't use
trivially blob like codec specific data.

Depends on D145519

Comment 8

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1747760 - P8: Handle Opus data using Opus codec specific variant. r?kinetik,padenot.

This means as of this patch

• All mp4 specific audio codecs are handled. There's some more we could
  theoretically get, stuff like ALAC, but it's not clear to me we handle them
  following demuxing. I've left a catch all in the mp4 demuxing code just in
  case.
• We no longer pack the codec-delay/preskip at the head of the opus binary blob.
  This means that the binary blob is just the opus header data and the container
  specific preskip has its own member. My hope is this is clearer and easier to
  understand. It also means we can drop some of the code we had for packing the
  delay/preskip into a binary blob.

Depends on D145520

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1747760 - P9: Handle Vorbis data with Vorbis codec specific variant. r?kinetik,padenot

Depends on D145521

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1747760 - P10: Remove AudioInfo.mExtraData. r?padenot,kinetik

This is no longer needed as the codec specific data member covers all codec
specific data.

Depends on D145522

Comment 11

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Try run of current patches: https://treeherder.mozilla.org/jobs?repo=try&revision=31b714af273b611bc043315b45d3e3173d1ab781

Looks mostly good. Some of the WPT failures make me nervous, but don't appear related to this.

Comment 12

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Big try run https://treeherder.mozilla.org/jobs?repo=try&revision=0a54e22cb6e86e9fa11a1ee988fc91ab22b20451

I've ironed out some remaining issues with different build targets, and raised some follow up items for stuff I wasn't 100% on how to address in the current stack.

Comment 13

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

For posterity and clarity -- these patches should not change any external behaviour. They should just be a refactor. My hope is that once these land, it will be easier to make functional changes to how we handle this metadata, but the goal here is just a refactor.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0b666cba2ce
P1: Add audio codec specific info variant to help give more type info for codec specific data. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/ad0284c66cb0
P2: Add IPC code for new codec types. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/0f6c6489a2c6
P3: Rework audio codec specific data to use new variant with a generic blob. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/71e0dd7f0dad
P4: Handle mp3 codec specific data using mp3 codec specific variant. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/4740c9cf6d51
P5: Handle flac codec specific data using flac codec specific variant. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/ede5ba8832cb
P6: Use Wave specific codec data for Wave audio. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/706c89c31a6d
P7: Handle aac codec specific data using aac codec specific variant. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/51ded936bda1
P8: Handle Opus data using Opus codec specific variant. r=kinetik.
https://hg.mozilla.org/integration/autoland/rev/dbb36eb962f7
P9: Handle Vorbis data with Vorbis codec specific variant. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/aae68dac17d0
P10: Remove AudioInfo.mExtraData. r=kinetik

Comment 15

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing wpt failures on mediasource-correct-frames-after-reappend.html

Backout link : https://hg.mozilla.org/integration/autoland/rev/97cc8b3b643665a8e59133e2d9d263369c0dc1bd

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=aae68dac17d07a01cafc3b3d89e3ccca44e04cd9&selectedTaskRun=PCnkxpyCT3yremw__9p0WA.0

Link to failure log : https://treeherder.mozilla.org/logviewer?job_id=377738116&repo=autoland&lineNumber=2756

Comment 16

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Thanks for heads up.

Notes while looking at failure -- the linux specific nature and the test that is failing suggests there is some aac data that I'm failing to appropriately pipe somewhere on a Linux specifc path. Ffmpeg maybe? Holding NI while I investigate.

Comment 17

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

It's patch 3 that starts the issue. Or at least makes it worse than on central.

The issue appears intermittent, as I can sometimes get tests to pass, per https://treeherder.mozilla.org/jobs?repo=try&revision=97fadcd8f218a4cb8235e3044b3bfa29379bb3cd

I can also intermittently repro the issue locally on Windows. So it may not be platform specific at all.

Comment 18

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Think I got it -- it's actually patch 1 that has the issue, but since we don't use the new structures until patch 3, that's where the problem crops up. The issue is that my operator== code for the structs with binary data was incorrect. I was doing the ole compare pointers rather than compare their contents. This was tripping up our checks for if we need to recreate decoders, so we were recreating decoders when we didn't need to. And this was causing the test failure we saw, I believe because that test is somewhat racy and the decoder recreation pushed us over the edge in terms of timing.

That said, the test being racy works for me if it helps catching my bugs. Fix incoming.

Comment 19

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Try run at https://treeherder.mozilla.org/jobs?repo=try&revision=ecec3d33c5b777407fefdf0cccc39f1c4a001073 looks good. I'm fairly confident I've clobbered the issue leading to the fail above. Let's try this again.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e525f667e341
P1: Add audio codec specific info variant to help give more type info for codec specific data. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/513063d8bb74
P2: Add IPC code for new codec types. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/e978acfec210
P3: Rework audio codec specific data to use new variant with a generic blob. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/3779014cbb83
P4: Handle mp3 codec specific data using mp3 codec specific variant. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/9bb877ae231e
P5: Handle flac codec specific data using flac codec specific variant. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/7e25b435499d
P6: Use Wave specific codec data for Wave audio. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/230d6d172edd
P7: Handle aac codec specific data using aac codec specific variant. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/dd1edaac22d4
P8: Handle Opus data using Opus codec specific variant. r=kinetik.
https://hg.mozilla.org/integration/autoland/rev/0ff888a82b21
P9: Handle Vorbis data with Vorbis codec specific variant. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/1e98fd258975
P10: Remove AudioInfo.mExtraData. r=kinetik

Comment 21

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/e525f667e341
https://hg.mozilla.org/mozilla-central/rev/513063d8bb74
https://hg.mozilla.org/mozilla-central/rev/e978acfec210
https://hg.mozilla.org/mozilla-central/rev/3779014cbb83
https://hg.mozilla.org/mozilla-central/rev/9bb877ae231e
https://hg.mozilla.org/mozilla-central/rev/7e25b435499d
https://hg.mozilla.org/mozilla-central/rev/230d6d172edd
https://hg.mozilla.org/mozilla-central/rev/dd1edaac22d4
https://hg.mozilla.org/mozilla-central/rev/0ff888a82b21
https://hg.mozilla.org/mozilla-central/rev/1e98fd258975