1104475 - Refactor Vorbis and Opus decoding out of WebMReader

Status: RESOLVED FIXED

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

Description

[Matthew Gregan [:kinetik]]

Bug 922314 refactored the VPx decoder out of WebMReader into a new video decoder class.  We should do the same thing with the Vorbis and Opus decoders.

A later step is to factor the same code out of the OggReader to reduce code duplication.

That'll make maintenance easier and make it easier to add Opus support to the MP4Reader at a later stage.

Note that I'm not actively working on this, simply poking at it as I have free time, so feel free to ping me for the latest patch and continue work if it interests you.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: wip v0

Builds, runs.

Comment 2

[Jan Gerber]

Attachment: 0002-Refactor-Vorbis-and-Opus-decoding-out-of-WebMReader.patch

update patch to work with current tree.

Comment 3

[Jan Gerber]

Attachment: 0001-Bug-1104475-Refactor-Vorbis-and-Opus-decoding-out-of.patch

move discard padding parsing to reader and pass as int64_t

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 8612859 [details] [diff] [review]
0001-Bug-1104475-Refactor-Vorbis-and-Opus-decoding-out-of.patch

Review of attachment 8612859 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 5

[Ralph Giles (:rillian)]

This needs rebasing on top of more recent changes. E.g. bugs 1165515, 1160695, 1173001, 1163223. It doesn't apply cleanly.

Comment 6

[Jan Gerber]

Attachment: 0001-Bug-1104475-Refactor-Vorbis-and-Opus-decoding-out-of.patch

rebased to latest m-c, lets try to get it in before it rots again.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8624675 [details] [diff] [review]
0001-Bug-1104475-Refactor-Vorbis-and-Opus-decoding-out-of.patch

Review of attachment 8624675 [details] [diff] [review]:
-----------------------------------------------------------------

now that the new MSE is completely integrated, very keen on getting this done asap.

::: dom/media/webm/AudioDecoder.cpp
@@ +47,5 @@
> +class VorbisDecoder : public WebMAudioDecoder
> +{
> +public:
> +  virtual nsresult Init();
> +  virtual void Shutdown();

passing by comments. needs "override" on all overridden member (and remove virtual)

@@ +76,5 @@
> +  // destructor is called before |Init|.
> +  memset(&mVorbisBlock, 0, sizeof(vorbis_block));
> +  memset(&mVorbisDsp, 0, sizeof(vorbis_dsp_state));
> +  memset(&mVorbisInfo, 0, sizeof(vorbis_info));
> +  memset(&mVorbisComment, 0, sizeof(vorbis_comment));

use PodZero.

@@ +132,5 @@
> +{
> +  MOZ_ASSERT(mPacketCount == 3);
> +
> +  int r = vorbis_synthesis_init(&mVorbisDsp, &mVorbisInfo);
> +  if (r != 0) {

if (r)

@@ +156,5 @@
> +  MOZ_ASSERT(mPacketCount >= 3);
> +  ogg_packet pkt = InitOggPacket(aData, aLength, false, false, -1, mPacketCount++);
> +  bool first_packet = mPacketCount == 4;
> +
> +  if (vorbis_synthesis(&mVorbisBlock, &pkt) != 0) {

don't use != 0

Comment 8

[Jan Gerber]

Attachment: 0001-Bug-1104475-Refactor-Vorbis-and-Opus-decoding-out-of.patch

new version of the patch addressing Jean-Yves comments

Comment 9

[Jan Gerber]

anyone up for landing this?

Comment 10

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd364bbbe253

Comment 11

[Matthew Gregan [:kinetik]]

Gladly!  Pushed to Try, link in previous comment.

Comment 12

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8445f122dec6

Comment 13

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=410eda30147e

Comment 14

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ae00df02ec

Comment 15

[Matthew Gregan [:kinetik]]

Landed with two trivial changes: explicit keyword on single arg AudioDecoder constructors and a NULL-check in ResetDecode.

Comment 16

[Matthew Gregan [:kinetik]]

Thanks for getting this over the finish line, Jan!

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7ae00df02ec