Closed Bug 1104475 Opened 5 years ago Closed 5 years ago

Refactor Vorbis and Opus decoding out of WebMReader

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: kinetik, Assigned: j)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch wip v0 (obsolete) — Splinter Review
Builds, runs.
Blocks: 1119208
Blocks: 1148102
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Priority: -- → P5
Priority: P5 → P3
Depends on: 1168732
update patch to work with current tree.
Attachment #8528159 - Attachment is obsolete: true
move discard padding parsing to reader and pass as int64_t
Attachment #8611056 - Attachment is obsolete: true
Attachment #8612859 - Flags: review?(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!
Attachment #8612859 - Flags: review?(kinetik) → review+
Assignee: kinetik → j
This needs rebasing on top of more recent changes. E.g. bugs 1165515, 1160695, 1173001, 1163223. It doesn't apply cleanly.
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
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)
Attachment #8624714 - Flags: review?(kinetik) → review+
anyone up for landing this?
Flags: needinfo?(kinetik)
Flags: needinfo?(giles)
Gladly!  Pushed to Try, link in previous comment.
Flags: needinfo?(kinetik)
Flags: needinfo?(giles)
Landed with two trivial changes: explicit keyword on single arg AudioDecoder constructors and a NULL-check in ResetDecode.
Thanks for getting this over the finish line, Jan!
https://hg.mozilla.org/mozilla-central/rev/e7ae00df02ec
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.