Refactor Vorbis and Opus decoding out of WebMReader

RESOLVED FIXED in Firefox 41

Status

()

Core
Audio/Video
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kinetik, Assigned: Jan Gerber)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8528159 [details] [diff] [review]
wip v0

Builds, runs.
Blocks: 946180
Blocks: 1119208
Blocks: 1148102
(Reporter)

Updated

3 years ago
Assignee: nobody → kinetik
Status: NEW → ASSIGNED

Updated

3 years ago
Priority: -- → P5

Updated

3 years ago
Priority: P5 → P3
(Assignee)

Updated

3 years ago
Depends on: 1168732
Blocks: 1168674
(Assignee)

Comment 2

3 years ago
Created attachment 8611056 [details] [diff] [review]
0002-Refactor-Vorbis-and-Opus-decoding-out-of-WebMReader.patch

update patch to work with current tree.
Attachment #8528159 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Created attachment 8612859 [details] [diff] [review]
0001-Bug-1104475-Refactor-Vorbis-and-Opus-decoding-out-of.patch

move discard padding parsing to reader and pass as int64_t
Attachment #8611056 - Attachment is obsolete: true
Attachment #8612859 - Flags: review?(kinetik)
(Reporter)

Comment 4

3 years ago
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!
Attachment #8612859 - Flags: review?(kinetik) → review+
(Reporter)

Updated

3 years ago
Assignee: kinetik → j
This needs rebasing on top of more recent changes. E.g. bugs 1165515, 1160695, 1173001, 1163223. It doesn't apply cleanly.
(Assignee)

Comment 6

3 years ago
Created attachment 8624675 [details] [diff] [review]
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.
Attachment #8612859 - Attachment is obsolete: true
Attachment #8624675 - Flags: review?(kinetik)
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
(Assignee)

Comment 8

3 years ago
Created attachment 8624714 [details] [diff] [review]
0001-Bug-1104475-Refactor-Vorbis-and-Opus-decoding-out-of.patch

new version of the patch addressing Jean-Yves comments
Attachment #8624675 - Attachment is obsolete: true
Attachment #8624675 - Flags: review?(kinetik)
Attachment #8624714 - Flags: review?(kinetik)
(Reporter)

Updated

3 years ago
Attachment #8624714 - Flags: review?(kinetik) → review+
(Assignee)

Comment 9

3 years ago
anyone up for landing this?
Flags: needinfo?(kinetik)
Flags: needinfo?(giles)
(Reporter)

Comment 10

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd364bbbe253
(Reporter)

Comment 11

3 years ago
Gladly!  Pushed to Try, link in previous comment.
Flags: needinfo?(kinetik)
Flags: needinfo?(giles)
(Reporter)

Comment 12

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8445f122dec6
(Reporter)

Comment 13

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=410eda30147e
(Reporter)

Comment 14

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ae00df02ec
(Reporter)

Comment 15

3 years ago
Landed with two trivial changes: explicit keyword on single arg AudioDecoder constructors and a NULL-check in ResetDecode.
(Reporter)

Comment 16

3 years ago
Thanks for getting this over the finish line, Jan!
https://hg.mozilla.org/mozilla-central/rev/e7ae00df02ec
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.