Closed Bug 674225 Opened 13 years ago Closed 12 years ago

support the Opus voice codec in <audio> and <video> elements

Categories

(Core :: Audio/Video, defect)

All
Other
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: beta, Assigned: rillian)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [mentor=rillian] [lang=c++])

Attachments

(4 files, 23 obsolete files)

1.89 MB, patch
derf
: review+
rillian
: checkin+
Details | Diff | Splinter Review
7.61 KB, patch
ted
: review+
rillian
: checkin+
Details | Diff | Splinter Review
28.38 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
15.62 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110622232440

Steps to reproduce:

From discussion at Bug 476752, opened this tracking bug for Opus codec support.
It seems advantageous for a number of applications (voip, gaming, presentations) to support a low bandwidth speech codec in HTML5 media elements.

Further information can be found at http://opus-codec.org/
Status: UNCONFIRMED → NEW
Ever confirmed: true
We do plan to integrate Opus as part of the webrtc stack. As John said on the Speex bug, Opus isn't standardized yet (nor are any container mappings) so it's premature to land it on trunk for the moment.
Attached patch Add build system support (obsolete) — Splinter Review
Just an update with preliminary work on this bug. These patches provide the reference implementation.

Still todo: Opus support in the Ogg reader, and the media elements
Whiteboard: [mentor=rillian] [lang=c++]
Update to the latest IETF draft.
Attachment #578448 - Attachment is obsolete: true
Attached patch Add opus to the build system (obsolete) — Splinter Review
Some updates from old work.
Attachment #578450 - Attachment is obsolete: true
Attached patch Add opus to nsOggReader (obsolete) — Splinter Review
Patch adding decode support. Linear playback works.

Still issues:

Playback stalls after seek with "###!!! ASSERTION: Must get a granuletime: 'granuleTime > 0', file /home/giles/mozilla/firefox/content/media/ogg/nsOggReader.cpp, line 1410."

We want to hide this behind a runtime pref, this patch is unconditional.

No tests.
Attachment #610223 - Attachment is patch: true
Status update: The current plan is to land this pref'd off in firefox 14.
Target Milestone: --- → mozilla14
Version: 5 Branch → Trunk
Comment on attachment 610223 [details] [diff] [review]
Add opus to nsOggReader

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

::: content/media/ogg/nsOggReader.cpp
@@ +412,5 @@
> +  PRUint32 channels = mOpusState->mChannels;
> +  nsAutoArrayPtr<AudioDataValue> buffer(new AudioDataValue[frames * channels]);
> +
> +  //TODO: handle int16 output if MOZ_TREMOR
> +  NS_ASSERTION(MOZ_SAMPLE_TYPE_FLOAT32, "need to hook up fixed-point decode");

The proper thing to do here is probably to make a generic #define for fixed-point audio decoders, instead of re-using MOZ_TREMOR for other codecs, since it has an impact on how we shuffle the decoded data around internally for all codecs.
Assignee: nobody → giles
Comment on attachment 610223 [details] [diff] [review]
Add opus to nsOggReader

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

::: content/media/ogg/nsOggCodecState.cpp
@@ +827,5 @@
> +    return NS_ERROR_FAILURE;
> +
> +  int count, preskip, rate, gain, mapping, streams;
> +  count = aPacket->packet[9];
> +  preskip = aPacket->packet[11] << 8 | aPacket->packet[10];

LEUint16

@@ +828,5 @@
> +
> +  int count, preskip, rate, gain, mapping, streams;
> +  count = aPacket->packet[9];
> +  preskip = aPacket->packet[11] << 8 | aPacket->packet[10];
> +  rate = aPacket->packet[12] |

Use LEUint32().

@@ +832,5 @@
> +  rate = aPacket->packet[12] |
> +         (aPacket->packet[13] << 8) |
> +         (aPacket->packet[14] << 16) |
> +         (aPacket->packet[15] << 24);
> +  gain = (aPacket->packet[17] << 8) | aPacket->packet[16];

LEUint16()
Comment on attachment 610223 [details] [diff] [review]
Add opus to nsOggReader

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

Seeking in Ogg fails... Who would've guessed? When seeking in Vorbis and Theora we (need to) call nsOggReader::ResetDecode() so that we call vorbis_synthesis_restart, and so that we clear all the undecoded packets from the old playback position, don't you need to do something similar for Opus? So add a call to mOpusState::Reset() to nsOggReader::ResetDecode(). You probably don't want to call opus_decoder_destroy in nsOpusState::Reset() then.

::: content/media/ogg/nsOggCodecState.cpp
@@ +847,5 @@
> +  mChannelMapping = mapping;
> +  mPreSkip = preskip;
> +  mGain = (float)gain / 256.0;
> +
> +  printf(" opus channel count %d\n", mChannels);

You can use LOG(PR_LOG_DEBUG, ("string",args...)) instead, and it'll not get scrambled by other threads' output. You need to run with NSPR_LOG_MODULES=nsBuiltinDecoder:5 environment variable to see it though.
Attached patch Add opus to nsOggReader (obsolete) — Splinter Review
Updated patch. Seeking works now. Thanks for the pointer, Chris.

I believe this addresses cpearce's comments, but not Tim's suggestion for fixed-point decoding. We also still need a pref.
Attachment #610223 - Attachment is obsolete: true
Attached patch Add opus to nsOggReader (obsolete) — Splinter Review
Minor robustness fix.

Please review. We don't work on android yet, but I'd like to start landing code anyway, since it will be behind a pref.
Attachment #611621 - Attachment is obsolete: true
Attachment #611659 - Flags: review?(cpearce)
New patch disabling decode support by default.
Attachment #611660 - Flags: review?(cpearce)
Attachment #607664 - Flags: review?(tterribe)
Attachment #607664 - Flags: review?(cpearce)
Attachment #607641 - Flags: review?(tterribe)
Comment on attachment 611659 [details] [diff] [review]
Add opus to nsOggReader

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

Getting there. Mostly style type changes to make, and it'll be r+.

::: content/media/ogg/nsOggCodecState.cpp
@@ +780,5 @@
>  }
>  
>  
> +nsOpusState::nsOpusState(ogg_page* aBosPage) :
> +    nsOggCodecState(aBosPage, true),

Use tab-width of 2 spaces please. While you're here, can you fix nsSkeletonState::nsSkeletonState to have the same formatting WRT ':' as the others (nsVorbisState and nsTheoraState) please?

@@ +790,5 @@
> +    MOZ_COUNT_CTOR(nsOpusState);
> +}
> +
> +nsOpusState::~nsOpusState() {
> +    MOZ_COUNT_DTOR(nsOpusState);

Indentation.

@@ +841,5 @@
> +    return NS_ERROR_FAILURE;
> +
> +  // try parsing as the metadata header
> +  if (!memcmp(aPacket->packet, "OpusTags", 8)) {
> +      mDoneReadingHeaders = true; // last opus header

Indentation.

@@ +847,5 @@
> +      return true;
> +  }
> +
> +  // otherwise, parse as the id header
> +  if (memcmp(aPacket->packet, "OpusHead\0", 9))

I'd fold these two if statements together into an if (a || b) return NS_ERROR_FAILURE, since they both return the same value.

@@ +855,5 @@
> +
> +  mRate = 48000; // decoder runs at 48 kHz regardless
> +
> +  mChannels= aPacket->packet[9];
> +  mPreSkip = LEUint16(aPacket->packet + 10);

mPreSkip is write only? Why do we need it then?

@@ +860,5 @@
> +  mNominalRate = LEUint32(aPacket->packet + 12);
> +  mGain = (float)LEUint16(aPacket->packet + 16) / 256.0;
> +  mChannelMapping = aPacket->packet[18];
> +
> +  int streams;

|streams| is write only?

@@ +876,5 @@
> +  if (granulepos < 0)
> +    return -1;
> +
> +  /* Ogg Opus always runs at a granule rate of 48 kHz */
> +  //CheckedInt64 t = CheckedInt64(granulepos - mPreSkip) * USECS_PER_S;

Remove commented out code.

@@ +878,5 @@
> +
> +  /* Ogg Opus always runs at a granule rate of 48 kHz */
> +  //CheckedInt64 t = CheckedInt64(granulepos - mPreSkip) * USECS_PER_S;
> +  // HACK: don't subtract preskip to avoid generating negative timestamps.
> +  CheckedInt64 t = CheckedInt64(granulepos) * USECS_PER_S;

Not sure about this whole HACK thing. Maybe you should be using NS_MAX(0, granulepos - mPreSkip) instead?

@@ +900,5 @@
> +{
> +  if (!mActive)
> +    return NS_OK;
> +  NS_ASSERTION(static_cast<PRUint32>(ogg_page_serialno(aPage)) == mSerial,
> +      "Page is not for this stream");

Line up second/continuation line(s) with opening '(', rather than 2 tabs in.
e.g.:
  
  SomeFunctionName(argument1,
                   argument2,
                   argument3,
                   ...);

(needs a fixed width font to look correct).

@@ +926,5 @@
> +{
> +  NS_ASSERTION(mUnstamped.Length() > 0, "Must have unstamped packets");
> +  ogg_packet* last = mUnstamped[mUnstamped.Length()-1];
> +  NS_ASSERTION(last->e_o_s || last->granulepos > 0,
> +      "Must know last granulepos!");

Line up second/continuation line(s) with opening '(', rather than 2 tabs in.

@@ +927,5 @@
> +  NS_ASSERTION(mUnstamped.Length() > 0, "Must have unstamped packets");
> +  ogg_packet* last = mUnstamped[mUnstamped.Length()-1];
> +  NS_ASSERTION(last->e_o_s || last->granulepos > 0,
> +      "Must know last granulepos!");
> +  if (mUnstamped.Length() == 1)

Your for loop does this check already, you don't need it...

@@ +936,5 @@
> +  // for the current packet.
> +  for (PRUint32 i = mUnstamped.Length() - 1; i > 0; i--) {
> +    ogg_packet* next = mUnstamped[i];
> +    int offset = opus_decoder_get_nb_samples(mDecoder,
> +        next->packet, next->bytes);

Line up second/continuation line(s) with opening '(', rather than 2 tabs in.

::: content/media/ogg/nsOggCodecState.h
@@ +336,5 @@
> +  bool IsHeader(ogg_packet* aPacket);
> +  nsresult PageIn(ogg_page* aPage);
> +
> +  int mRate;
> +  int mNominalRate;

Need comments on what these mean.

::: content/media/ogg/nsOggReader.cpp
@@ +418,5 @@
> +  //TODO: handle int16 output if MOZ_TREMOR
> +  NS_ASSERTION(MOZ_SAMPLE_TYPE_FLOAT32, "need to hook up fixed-point decode");
> +
> +  int ret = opus_decode_float(mOpusState->mDecoder,
> +        aPacket->packet, aPacket->bytes, buffer, frames, false);

Line up continuation with '(' from previous line.

@@ +440,5 @@
>  bool nsOggReader::DecodeAudioData()
>  {
>    NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
> +  NS_ASSERTION(mVorbisState!=0 || mOpusState!=0,
> +    "Need audio codec state to decode audio");

Line up continuation with '(' from previous line. And we should be using nsnull instead of 0 as null pointers (I made that mistake elsewhere in this file, maybe you could fix them while you're here?)

@@ +445,4 @@
>  
>    // Read the next data packet. Skip any non-data packets we encounter.
>    ogg_packet* packet = 0;
> +  if (mVorbisState) {

Code duplication is bad. How about:

nsOggCodecState* codecState = mVorbisState ? mVorbisState : mOpusState;
do {
  if (packet) {
    nsOggCodecState::ReleasePacket(packet);
  }
  packet = NextOggPacket(codecState);
} while (packet && codecState->IsHeader(packet));

@@ +467,5 @@
>  
>    NS_ASSERTION(packet && packet->granulepos != -1,
>      "Must have packet with known granulepos");
>    nsAutoReleasePacket autoRelease(packet);
> +  if (mVorbisState)

if (mVorbisState) {
  DecodeVorbis(packet);
} else if (mOpusState) {
  DecodeOpus(packet);
}

(Since you check mOpusState != null above.)
(You can omit brackets in if statement's blocks on trival if/return patterns (like |if (condition) return NS_ERROR_FAILURE;|), but any block which may get code added to it should have brackets on it.)
Attachment #611659 - Flags: review?(cpearce) → review-
Comment on attachment 611660 [details] [diff] [review]
Hide Opus decoding behind media.opus.enable

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

Almost there.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1870,5 @@
>    }
>  #endif
>  #ifdef MOZ_OGG
>    if (IsOggType(nsDependentCString(aMIMEType))) {
>      *aCodecList = gOggCodecs;

How about you make IsOpusEnabled() visible if MOZ_OGG is defined, and have it return false if MOZ_OPUS is not defined, otherwise return the pref.

The this can be:
*aCodecList = IsOpusEnabled() ? gOggCodecsWithOpus : gOggCodecs;

And the compile error I pointed out in nsOggReader::nsOggReader won't happen.

::: content/media/ogg/nsOggReader.cpp
@@ +106,5 @@
>    : nsBuiltinDecoderReader(aDecoder),
>      mTheoraState(nsnull),
>      mVorbisState(nsnull),
>      mOpusState(nsnull),
> +    mOpusEnabled(nsHTMLMediaElement::IsOpusEnabled()),

This will fail to compile when MOZ_OPUS is not defined, since IsOpusEnabled() won't be defined.

@@ +229,5 @@
>        {
> +        if (mOpusEnabled)
> +          mOpusState = static_cast<nsOpusState*>(codecState);
> +        else
> +          printf("media.opus.enabled is not set\n");

So... in general printf's aren't visible in release builds so if you want to notify the user, you should write to the webconsole instead. For example: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#8743

If you want to notify developers by writing to stdout, you can use NS_WARNING() or LOG() (as defined on line 55).

If you do neither of these things, move the mOpusEnabled check up into the preceding if statement.
Attachment #611660 - Flags: review?(cpearce) → review-
Comment on attachment 611659 [details] [diff] [review]
Add opus to nsOggReader

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

Please can you also add an Opus audio file to content/media/test and its metadata to content/media/test/manifest.js, and run the unit tests locally and on TryServer (just enable it for the push). Please add the opus file to gSeekTests, gSmallTests, and gPlayTests in manifest.js.
Comment on attachment 607664 [details] [diff] [review]
Add opus to the build system

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

Push to try to test on all platforms. Ensure it's green across the board before asking for review please.

Add a block to about:license for including the external code, for example see libogg's Xiph.org umbrella license block in:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/license.html?force=1#2619
(Maybe you just need to add libopus to that?)

::: media/libopus/README_MOZILLA
@@ +1,4 @@
> +Using the draft source code, extracted from
> +https://tools.ietf.org/id/draft-ietf-codec-opus-11.txt
> +
> +Removed the upstream Makefile (Makefile.draft in git)

You should probably add the git revision you included, so we know exactly what we're shipping.
Attachment #607664 - Flags: review?(cpearce) → review-
Comment on attachment 607664 [details] [diff] [review]
Add opus to the build system

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

Also, you should get a build peer, like khuey or ted to review this too.
Comment on attachment 607664 [details] [diff] [review]
Add opus to the build system

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

A couple of other things that don't have a file/linenumber to which I can attach them:

Around configure.in:5591, there's a simple test that always enables tremor on arm. You can crib from that to do the same thing for Opus's fixed-point support (but outside the 'if test -n "$MOZ_OGG"' block, as we'll want Opus to work with Ogg disabled for WebRTC).

You also need a few lines in layout/media/Makefile.in to add libopus to gkmedias.dll on Windows. In addition, you'll need to put the public API symbols in symbols.def.in. The EXPORTS list in the Makefile only controls gcc visibility options, IIRC. Yes, this is kind of a mess (that describes the gkmedias thing in general).

::: media/libopus/Makefile
@@ -1,1 @@
> -#################### COMPILE OPTIONS #######################

Not sure where this came from, but this file never should have been added in the first place.

::: media/libopus/Makefile.in
@@ +46,5 @@
> +LIBRARY_NAME = opus
> +FORCE_STATIC_LIB= 1
> +
> +DEFINES += \
> +  -DUSE_ALLOCA -DHAVE_LRINTF -Drestrict= \

If you want to use -DUSE_ALLOCA (and you do), you're going to need a block like:

ifeq ($(OS_ARCH),AIX)
DEFINES += -Dalloca=__alloca
endif

(see media/libvorbis/lib/Makefile.in or media/libtremor/lib/Makefile.in for examples)

@@ +51,5 @@
> +  -DOPUS_BUILD \
> +  -DOPUS_VERSION='"draft-10-mozilla"' \
> +  $(NULL)
> +
> +EXPORTS = \

For all the other codecs (theora, tremor, vorbis, vp8), we use the pattern (e.g., for theora):

EXPORTS_NAMESPACES = theora

EXPORTS_theora = \
 ...

I don't know why (and this is why I'm not a build peer), but I'm guessing you should follow the same pattern.
Attachment #607664 - Flags: review?(tterribe) → review-
Comment on attachment 607641 [details] [diff] [review]
add the opus draft-11 source to the tree

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

You need an update.sh similar to what the other codecs use. Feel free to bikeshed on an easier way to pull in new code updates (jesup has scripts for WebRTC it may be useful to crib off of), but at the very least we need it to maintain documentation on what external patches we have, and there's value in doing this the same way as we do for all the other codecs. I realize there are no external patches right now, but I don't think we want to go through the process of adding such a thing and getting it reviewed when we want to add a hotfix for some security vulnerability. If we have to do that someday, we'll want to be able to land small one- or two-liners that we can get easy approval for on aurora and beta.

There're two good options: make update.sh part of the build system patch, so that this patch becomes solely the stuff introduced by running update.sh, or move both update.sh and README_MOZILLA to this patch. Either is fine with me, but in both cases there are some extra files you should not add:

Makefile
test/run_vectors.sh
src/opus_compare.c
src/opus_demo.c

Once you do this, you'll get an r+ from me, since I've spent the last year and a half (at least) reviewing all the code here that I didn't write myself, so it should at long last be good to go.
Attachment #607641 - Flags: review?(tterribe) → review-
Attached patch Add opus to nsOggReader (obsolete) — Splinter Review
Updated decoder patch in response to Chris' review comments.

Responses to comment #14 inline:

> Use tab-width of 2 spaces please. While you're here, can
> you fixnsSkeletonState::nsSkeletonState to have the same
> formatting WRT ':' as the others (nsVorbisState and nsTheoraState) please?

Hopefully all fixed.

> I'd fold these two if statements together into an if (a || b) return NS_ERROR_FAILURE, since they both return the same value.

Ok.

> mPreSkip is write only? Why do we need it then?

mPreSkip, mGain, samples and the channel map are all needed for features the patch doesn't implement. I intend to do so in follow up bugs after this lands. I also find it helpful when debugging to have the whole header parsed, but if you prefer I can remove these fields until we do something with them.

> Not sure about this whole HACK thing. Maybe you should be using NS_MAX(0, granulepos - mPreSkip) instead?

We're supposed to trim mPreSkip samples off the decoder's initial output after a reset, to allow the decoder's predicters to converse. We're also supposed to subtract the same number from the granulepos when calculating the timestamp so the stream starts at zero.

I don't implement this yet. I've just removed the code and comment for now.

>> +  if (mUnstamped.Length() == 1)
> 
> Your for loop does this check already, you don't need it...

Good point. Removed.

>> +  int mRate;
>> +  int mNominalRate;
> 
> Need comments on what these mean.

Documented.

> nsOggCodecState* codecState = mVorbisState ? mVorbisState : mOpusState;

Doing this requires a static_cast, so I when with an if (mVorbisState) codecState = ...;

Really, instead of DecodeVorbis and DecodeOpus we should have a Decode method on the various nsOggCodecState subclasses, but that's a more invasive change.

And comment #15

I've merged the prefs patch into the decode support patch.

> How about you make IsOpusEnabled() visible if MOZ_OGG is defined, and have it return false if MOZ_OPUS is not defined, otherwise return the pref.

Excellent idea. Thank you.

> > +          printf("media.opus.enabled is not set\n");

Oops. I've replaced this with an NS_WARNING for now.


I will add tests, address Tim's comments, and get some try results before requesting review again.
Attachment #611659 - Attachment is obsolete: true
Attachment #611660 - Attachment is obsolete: true
Attached audio Short opus test file (obsolete) —
'git hgp' didn't include the binary test file in the previous attachment. This goes is content/media/test/ along with opus-test.patch.
Updated source patch. This removes some unnecessary files, and includes the update.sh and README_MOZILLA files as suggested.
Attachment #607641 - Attachment is obsolete: true
Attached patch Add opus to the build system (obsolete) — Splinter Review
Update the build system patch in response to Tim's commments.
Attachment #607664 - Attachment is obsolete: true
Attached patch Add opus to nsOggReader (obsolete) — Splinter Review
Update the decoder patch for the opus.h namespace change in the most recent build patch.
Attachment #611963 - Attachment is obsolete: true
Just as a general update: I'm working through various tryserver issues now. Android and B2G failed to build without short AudioData support. I've fixed that locally.

However, a number of the media tests fail with the patches. E.g. timeouts on content/media/test/test_buffered.html and a number of others.
(In reply to Ralph Giles (:rillian) from comment #27)
> However, a number of the media tests fail with the patches. E.g. timeouts on
> content/media/test/test_buffered.html and a number of others.

Do you have a tryserver link for that push? There are known failures with test_buffered, and a few others, it's possible you're hitting them rather than a failure you've introduced...
I was seeing the timeouts with local runs on linux and mac, but hadn't done a try push with tests yet because the windows build is broken. It was repeatable for me with the patch, and not without, but I'd be happy to hear it's not my fault.

I've now pushed

 https://tbpl.mozilla.org/?tree=Try&rev=33a50bad916b

for reference.
Try run for 2aa02fcdd387 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2aa02fcdd387
Results (out of 2 total builds):
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rgiles@mozilla.com-2aa02fcdd387
Try run for 33a50bad916b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=33a50bad916b
Results (out of 114 total builds):
    success: 92
    warnings: 20
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rgiles@mozilla.com-33a50bad916b
 Timed out after 12 hours without completing.
Try run for a27887542149 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a27887542149
Results (out of 2 total builds):
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rgiles@mozilla.com-a27887542149
 Timed out after 12 hours without completing.
Attachment #614508 - Flags: review?(tterribe)
Comment on attachment 613397 [details] [diff] [review]
Add opus draft-11 source to the tree

Updating the source patch to match my current tree. The only change is r=derf in the commit message.
Attachment #613397 - Attachment is obsolete: true
Attached patch Add opus to the build system (obsolete) — Splinter Review
Updated build patch. This fixes issues on MSVC and Android.
Attachment #613398 - Attachment is obsolete: true
Attached patch Add opus to nsOggReader (obsolete) — Splinter Review
Updating patches to match my current tree.

The new decoder patch supports stripping the initial 'preskip' samples from the stream and accurate timestamp calculation.

Also adds the .opus filename extension to our uri loader to simplify testing.
Attachment #613399 - Attachment is obsolete: true
Update the mochitest patch to match my current tree.

The test file is trimmed to an exact duration in decimal seconds, and the opus filename extension is added to the mochitest http server and allowed file list.
Attachment #612358 - Attachment is obsolete: true
Attachment #612361 - Attachment is obsolete: true
New patch set pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=dbf42684fe7c
Comment on attachment 614508 [details] [diff] [review]
Add opus draft-11 source to the tree

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

r+ with a few minor quibbles.

::: media/libopus/update.sh
@@ +8,5 @@
> +TARGET='.'
> +
> +STATIC_FILES="COPYING"
> +MK_FILES="opus_sources.mk celt_sources.mk silk_sources.mk \
> +          opus_headers.txt celt_headers.txt silk_headers.txt"

opus_headers.txt, celt_headers.txt, and silk_headers.txt aren't actually added by this patch, but are copied into the ${TARGET} directory below?

@@ +23,5 @@
> +SRC_FILES=$(sed -e ':a;N;$!ba;s/\\\n//g;s/[A-Z_]* = //g' \
> +             $(for file in ${MK_FILES}; do echo "$1/${file}"; done))
> +
> +# pre-release versions of the code don't list opus_custom.h
> +# in celt_headers.mk, so we must include it manually

It's celt_headers.txt, not celt_headers.mk, for the version of the source you're importing now (though it will be celt_headers.mk in the next version, so if you don't want to make the comment correct now, that's fine).

@@ +45,5 @@
> +  echo ${cmd}
> +  ${cmd}
> +done
> +
> +# query git for the revision we're copying from

This is very nice.
Attachment #614508 - Flags: review?(tterribe) → review+
Thanks. You're correct that I forgot to add the _headers files to the patch. However, they're not required to build. We'll be updating to a newer release after this lands, so if the current patch builds green I'd like to land as-is and correct the quibbles then.
(In reply to Ralph Giles (:rillian) from comment #40)
> Thanks. You're correct that I forgot to add the _headers files to the patch.
> However, they're not required to build. We'll be updating to a newer release
> after this lands, so if the current patch builds green I'd like to land
> as-is and correct the quibbles then.

I am fine with that.
Comment on attachment 614511 [details] [diff] [review]
Add opus to the build system

Builds successfully on all platforms. Please review the build patch so we can land the opus codec source.

The mochitest-plain orange is from inconsistent media.duration values returned from the nsOggReader patch. The timestamp fix in the latest version didn't resolve this. I'll keep looking.
Attachment #614511 - Flags: review?(tterribe)
Attachment #614511 - Flags: review?(ted.mielczarek)
The problem I was having with test_buffered.html in the mochitest is because the patch doesn't trim the output of the final packet to patch the declared duration. This results in the duration attribute being bumped when the last packet is played.

However, we don't do this for any other Ogg files either. This is bug 616292. It's easier to trigger with opus files because the preskip header opusenc applies makes even round-number durations not end on packet boundaries.

In consulation with Chris Pearce, I'm going to work around this issue with test file which decodes to an integer number of packets. This will let us avoid orange until the issue can by fixed comprehensively.

I've pushed https://tbpl.mozilla.org/?tree=Try&rev=38c6c5849f99 with that modification. If there are no further issues, I'll update the test patch for final review.
Attached patch Add opus to nsOggReader (obsolete) — Splinter Review
Two minor updates to the decoder:

- guard nsOpusState::Time() against calls before the preskip header has been read

- pass the trimmed buffer length when submitting a trimmed buffer.

Thanks to cdielh for noticing out the second patch hadn't been included.
Attachment #614513 - Attachment is obsolete: true
Update the mochitest file to end on a packet boundary.

Works around nsOggReader not trimming decoder output to match the measured duration. See bug 616292.
Attachment #614524 - Attachment is obsolete: true
Attachment #616270 - Flags: review?(cpearce)
Attachment #616278 - Flags: review?(cpearce)
The intermittant 'leak running tests' on WinXP debug seems to be bug 723832.
Comment on attachment 616270 [details] [diff] [review]
Add opus to nsOggReader

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

Getting there. :)

You should try building without MOZ_OPUS defined and see what happens. ;)

Can you please make your comments start with a capital letter and end with full stop? Thanks.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1875,5 @@
>    }
>  #endif
>  #ifdef MOZ_OGG
>    if (IsOggType(nsDependentCString(aMIMEType))) {
>      *aCodecList = gOggCodecs;

*aCodecList = IsOpusEnabled() ? gOggCodecsWithOpus : gOggCodecs;

::: content/media/ogg/nsOggCodecState.cpp
@@ +85,5 @@
>    if (aPage->body_len > 6 && memcmp(aPage->body+1, "theora", 6) == 0) {
>      codecState = new nsTheoraState(aPage);
>    } else if (aPage->body_len > 6 && memcmp(aPage->body+1, "vorbis", 6) == 0) {
>      codecState = new nsVorbisState(aPage);
> +  } else if (aPage->body_len > 8 && memcmp(aPage->body, "OpusHead", 8) == 0) {

Won't this mean you'll instantiate an nsOpusState if a file contains an Opus stream, regardless of what nsHTMLMediaElement::IsOpusEnabled() returns? Oh, you just destroy it again in nsOggReader::ReadMetadata. I guess that's ok.

@@ +776,5 @@
>    return NS_OK;
>  }
>  
>  
> +nsOpusState::nsOpusState(ogg_page* aBosPage) :

opus.h is only included if MOZ_OPUS is defined, so this won't compile without MOZ_OPUS. As per the comment I made in nsOggCodecState.h, just wrap the definition of nsOpusState's member functions in #ifdef MOZ_OPUS (unless you can think of a better way).

@@ +804,5 @@
> +{
> +  nsresult res = NS_OK;
> +
> +  if (!mActive) {
> +    res = NS_ERROR_FAILURE;

I'm not sure it makes sense to return NS_ERROR_FAILURE if !mActive, the other codec states don't do this.

@@ +827,5 @@
> +
> +  NS_ASSERTION(mDecoder == NULL, "leaking OpusDecoder");
> +
> +  mDecoder = opus_decoder_create(mRate, mChannels, &error);
> +  if (error != OPUS_OK)

return error == OPUS_OK;

@@ +837,5 @@
> +bool nsOpusState::DecodeHeader(ogg_packet* aPacket)
> +{
> +  // minimum length of any header
> +  if (aPacket->bytes < 16)
> +    return NS_ERROR_FAILURE;

The return type of this function is bool, but you're returning an nsresult here. I think you should instead do:

if (aPacket->bytes < 16) {
  mActive = false;
  return true;
}

@@ +848,5 @@
> +  }
> +
> +  // otherwise, parse as the id header
> +  if (aPacket->bytes < 19 || memcmp(aPacket->packet, "OpusHead\0", 9))
> +    return NS_ERROR_FAILURE;

Same here, mActive=false;return true;

@@ +859,5 @@
> +  mGain = (float)LEUint16(aPacket->packet + 16) / 256.0;
> +  mChannelMapping = aPacket->packet[18];
> +
> +  int streams;
> +  if (mChannelMapping > 0 && aPacket->bytes > 19)

Does the !(mChannelMapping > 0 && aPacket->bytes > 19) path mean we're playing an invalid opus file?

@@ +875,5 @@
> +    return -1;
> +
> +  /* Ogg Opus always runs at a granule rate of 48 kHz */
> +  CheckedInt64 t = CheckedInt64(granulepos - mPreSkip) * USECS_PER_S;
> +  if (!t.valid())

return t.valid() ? t.value / mRate : -1;

(Saves extra lines.)

@@ +880,5 @@
> +    return -1;
> +  return t.value() / mRate;
> +}
> +
> +bool nsOpusState::IsHeader(ogg_packet* aPacket)

return aPacket->bytes >= 16 &&
       (!memcmp(aPacket->packet, "OpusHead\0", 9) ||
        !memcmp(aPacket->packet, "OpusTags", 8));

(Save lines.)

@@ +902,5 @@
> +    return NS_ERROR_FAILURE;
> +
> +  bool haveGranulepos;
> +  nsresult rv = PacketOutUntilGranulepos(haveGranulepos);
> +  if (NS_FAILED(rv))

if (NS_FAILED(rv) || haveGranulepos)\n return rv;

(Early return saves indenting block below.)

@@ +934,5 @@
> +                                             next->packet,
> +                                             next->bytes);
> +    // check for error (negative offset) and overflow
> +    if (offset > 0 && offset < next->granulepos)
> +      mUnstamped[i - 1]->granulepos = next->granulepos - offset;

else
  mUnstamped[i - 1]->granulepos = 0;

????

We should always try to get a granulepos for every packet, otherwise nsHTMLMediaElement.GetBuffered() will get confused.

::: content/media/ogg/nsOggCodecState.h
@@ +343,5 @@
> +  int mPreSkip;     // Number of samples to strip after decoder reset
> +  float mGain;      // Gain (dB) to apply to decoder output
> +  int mChannelMapping; // Channel mapping family
> +
> +  OpusDecoder *mDecoder;

Won't this fail to compile if MOZ_OPUS is not defined (and thus opus.h won't be included)? Perhaps you should wrap the class's members' declaration in #ifdef MOZ_OPUS (so the class declaration is empty when MOZ_OPUS isn't defined).

Or maybe declare struct OpusDecoder ifndef MOZ_OPUS or somesuch.

::: content/media/ogg/nsOggReader.cpp
@@ +432,5 @@
> +                        buffer, frames, false);
> +#endif
> +  if (ret < 0)
> +    return NS_ERROR_FAILURE;
> +  NS_ASSERTION(ret == frames, "Opus decoded too few audio samples");

If ret!=frames, is the opus file invalid?

If ret<frames we'll end up playing uninitialized memory in buffer (presumably), i.e. garbage, resulting in a click sound. If you use |ret| instead of |frames| below, in the ret<frames case the nsBuiltinDecoderStateMachine should insert silence *before* this AudioData's samples when it comes to play this AudioData in order play across the gap.

If you think silence should be injected *after* this packet's samples in the ret<frames case, then I suggest you explicitly null out the remainder of the buffer in this case.

What if ret>frames, what should you do then? Trim samples?

Never underestimate the ability of tools to produce invalid files.

@@ +446,5 @@
> +  if (startTime < 0) {
> +    PRInt32 skip = mOpusState->mPreSkip;
> +    PRInt32 goodFrames = frames - skip;
> +    NS_ASSERTION(goodFrames > 0, "endTime calculation was wrong");
> +    nsAutoArrayPtr<AudioDataValue> goodBuffer(new AudioDataValue[goodFrames * channels]);

How about passing buffer.forget()+skip*channels to the AudioData constructor rather than making a copy here? You'll need to adjust the other parameters of course.
Attachment #616270 - Flags: review?(cpearce) → review-
Comment on attachment 616278 [details] [diff] [review]
Add a short opus file to mochitest

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

r=cpearce provided you move bogus.duh to the end of the test arrays.

::: content/media/test/manifest.js
@@ +8,5 @@
>    { name:"small-shot.ogg", type:"audio/ogg", duration:0.276 },
>    { name:"r11025_s16_c1.wav", type:"audio/x-wav", duration:1.0 },
>    { name:"320x240.ogv", type:"video/ogg", width:320, height:240, duration:0.233 },
>    { name:"seek.webm", type:"video/webm", duration:3.966 },
> +  { name:"bogus.duh", type:"bogus/duh" },

Leave bogus.duh as the last entry in these arrays please.
Attachment #616278 - Flags: review?(cpearce) → review+
Comment on attachment 616270 [details] [diff] [review]
Add opus to nsOggReader

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

::: content/media/ogg/nsOggReader.cpp
@@ +432,5 @@
> +                        buffer, frames, false);
> +#endif
> +  if (ret < 0)
> +    return NS_ERROR_FAILURE;
> +  NS_ASSERTION(ret == frames, "Opus decoded too few audio samples");

> If ret!=frames, is the opus file invalid?

It's not that the file's invalid. Both opus_decoder_get_nb_samples() and opus_decode[_float]() parse the same packet to figure out how many samples to return. If they differ, that means there is a bug in libopus. The only way it _should_ be possible for ret!=frames is if there's an error during decoding, in which case you'll hit the ret<0 test above.

That said, the libopus code responsible for guaranteeing this property holds is very convoluted (I spent a good 45 minutes staring at it before I convinced myself it was true, and managed to read it the wrong two different ways first), so having a bug there is possible.
(In reply to Timothy B. Terriberry (:derf) from comment #50)
> Comment on attachment 616270 [details] [diff] [review]
> > If ret!=frames, is the opus file invalid?
> 
> It's not that the file's invalid. Both opus_decoder_get_nb_samples() and
> opus_decode[_float]() parse the same packet to figure out how many samples
> to return. If they differ, that means there is a bug in libopus. 

Could this be caused by a bug in encoding? I'm more worried about metadata stored in a file not matching reality, which was a problem we encountered a lot with Ogg files in the past (typically ffmpeg or oggzchop muxed).
(In reply to Chris Pearce (:cpearce) from comment #51)
> Could this be caused by a bug in encoding? I'm more worried about metadata
> stored in a file not matching reality, which was a problem we encountered a
> lot with Ogg files in the past (typically ffmpeg or oggzchop muxed).

No, there's nothing to be inconsistent with, here. The TOC sequence at the start of the packet is the only description of how much audio is contained in that individual packet. There are the usual granule positions at the page level, but that's not what Ralph is checking, here.
Ok, just leave it as an assertion then.
> NS_ASSERTION(ret == frames, "Opus decoded too few audio samples");

I was using an assertion because the API says it returns the number of samples decoded here. As Tim says, something is very wrong if this happens, so I wanted the guard to catch if libopus stopped behaving as expected.
Update the patch to leave the 'bogus.duh' dummy test filename at the end of the arrays, as requested in comment #49.
Attachment #616278 - Attachment is obsolete: true
Attachment #616869 - Flags: review+
Comment on attachment 614511 [details] [diff] [review]
Add opus to the build system

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

r=me with a few fiddly formatting nits.

::: layout/media/Makefile.in
@@ +73,5 @@
>  
> +ifdef MOZ_OPUS
> +SHARED_LIBRARY_LIBS	+= \
> +	$(DEPTH)/media/libopus/$(LIB_PREFIX)opus.$(LIB_SUFFIX) \
> +	$(NULL)

I know the rest of this file doesn't follow it, but our Makefile style is to use two-space indent after continuations. Also, ditch the literal tab after the SHARED_LIBRARY_LIBS. (Generally, no literal tabs anywhere except in rule commands where they're necessary.)

::: media/libopus/Makefile.in
@@ +31,5 @@
> +# and other provisions required by the GPL or the LGPL. If you do not delete
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****

You can use the shorter MPLv2 boilerplate for new files:
http://www.mozilla.org/MPL/headers/

@@ +46,5 @@
> +LIBRARY_NAME = opus
> +FORCE_STATIC_LIB= 1
> +
> +DEFINES += \
> +  -DUSE_ALLOCA -Drestrict= \

If you're going to use continuations, you might as well list them one-per-line.

@@ +93,5 @@
> +  $(notdir $(SILK_SOURCES)) \
> +  $(notdir $(OPUS_SOURCES)) \
> +  $(NULL)
> +
> +ifdef MOZ_OPUS_FIXED

Does this actually get defined anywhere, or is it just for testing purposes?

::: toolkit/toolkit-tiers.mk
@@ +135,5 @@
>  
> +ifdef MOZ_OPUS
> +tier_platform_dirs += \
> +		media/libopus \
> +		$(NULL)

Please ignore the rest of this file and use two-space indent after a continuation.
Attachment #614511 - Flags: review?(ted.mielczarek) → review+
(In reply to Chris Pearce (:cpearce) from comment #48)
> Comment on attachment 616270 [details] [diff] [review]
> Add opus to nsOggReader

Thanks for the review.

> Can you please make your comments start with a capital letter and end with
> full stop? Thanks.

Ok.

> >    if (IsOggType(nsDependentCString(aMIMEType))) {
> >      *aCodecList = gOggCodecs;
> 
> *aCodecList = IsOpusEnabled() ? gOggCodecsWithOpus : gOggCodecs;

Thanks.

> > +  } else if (aPage->body_len > 8 && memcmp(aPage->body, "OpusHead", 8) == 0) {
> 
> Won't this mean you'll instantiate an nsOpusState if a file contains an Opus
> stream, regardless of what nsHTMLMediaElement::IsOpusEnabled() returns? Oh,
> you just destroy it again in nsOggReader::ReadMetadata. I guess that's ok.

Yes. We could check for enabled here too, but then it doesn't fit on one line. Not any messier than not using ::IsHeader() in the first place, I think.

> > +nsOpusState::nsOpusState(ogg_page* aBosPage) :
> 
> opus.h is only included if MOZ_OPUS is defined, so this won't compile
> without MOZ_OPUS. As per the comment I made in nsOggCodecState.h, just wrap
> the definition of nsOpusState's member functions in #ifdef MOZ_OPUS (unless
> you can think of a better way).

Did you mean to comment out the bodies of the definitions? These functions are called from several places in nsOggReader, so I either need to provide dummy definitions, or add a *lot* of #ifdef'd code.

What about removing the MOZ_OPUS conditional entirely? We have a bunch of these in the media code, and I don't think it's worth the hassle of maintaining them. How regularly are all the combinations tested? We have the runtime checks now.

>> nsOpusState::Reset
>
> I'm not sure it makes sense to return NS_ERROR_FAILURE if !mActive, the
> other codec states don't do this.

You're right. It doesn't look like it's necessary. Does it make sense to still check this before actually resetting the codec? nsVorbisState::Reset does this as a side effect of using mActive as a proxy for initialization.

> return error == OPUS_OK;

I find that harder to read and step through, but ok.

> if (aPacket->bytes < 16) {
>   mActive = false;
>   return true;

Thanks. What do 'true' and 'false' mean here? Do we need to return true on failure because we want to fail silently?

> Does the !(mChannelMapping > 0 && aPacket->bytes > 19) path mean we're
> playing an invalid opus file?

No, If mChannelMapping is zero, the stream mapping is trivial and a decoder table for it isn't included in the header. If mChannelMapping is greater than zero, there MUST be a stream mapping table starting at the 19th byte. So the file is only invalid if one, but not both, of those conditions is true.

This is part of the surround support, which is bug 748144. I just wanted to have the code in there for validating headers. Would you like me to add a reject for the invalid case?

> return t.valid() ? t.value / mRate : -1;

Ok.

> return aPacket->bytes >= 16 &&
>        (!memcmp(aPacket->packet, "OpusHead\0", 9) ||
>         !memcmp(aPacket->packet, "OpusTags", 8));

Ok.

> @@ +902,5 @@
> > +    return NS_ERROR_FAILURE;
> > +
> > +  bool haveGranulepos;
> > +  nsresult rv = PacketOutUntilGranulepos(haveGranulepos);
> > +  if (NS_FAILED(rv))
> 
> if (NS_FAILED(rv) || haveGranulepos)\n return rv;

You mean 'if (NS_FAILED(rv) || !haveGranulepos)' ?
Would 'if !(NS_SUCCEEDED(rv) || haveGranulepos)" be better?

> @@ +934,5 @@
> > +                                             next->packet,
> > +                                             next->bytes);
> > +    // check for error (negative offset) and overflow
> > +    if (offset > 0 && offset < next->granulepos)
> > +      mUnstamped[i - 1]->granulepos = next->granulepos - offset;
> 
> else
>   mUnstamped[i - 1]->granulepos = 0;
> 
> ????
> 
> We should always try to get a granulepos for every packet, otherwise
> nsHTMLMediaElement.GetBuffered() will get confused.

It will be less confused by clamping to zero than by a -1?

I was thinking about preventing overflow here, but this may actually conflict with the trim code in nsOggReader::DecodeOpus. But if they have to be non-negative, clamping to zero isn't worse than any of the other failure modes. Any file where this occurs is technically invalid.

> How about passing buffer.forget()+skip*channels to the AudioData constructor
> rather than making a copy here? You'll need to adjust the other parameters
> of course.

If that works, that would be a lot simpler. I don't understand how buffer's lifetime is managed here, so I was trying to construct things the same way.
Blocks: 748144
> How about passing buffer.forget()+skip*channels to the AudioData constructor

Follow up based on irc discussion: this won't work because the MediaQueue needs the correct base pointer to be able to free the AudioDataValue array later.

Alternatives to reallocation are to add an AudioData constructor with an offset argument and propagate that through the code, or to use the current buffer, but memmove the valid sample range to the start of the buffer.

This code is and uncommon path, typically applying to one or two packets at the start of the stream, so the allocation bandwidth isn't an issue. I'll leave it as is.
nsBuiltinDecoderReader::DecodeToTarget does something similar around line 365.
It'd kinda be nice to push the decode specific stuff down into the codec states, but it's not really worth the hassle at this stage I'd say.

(In reply to Ralph Giles (:rillian) from comment #57)
> (In reply to Chris Pearce (:cpearce) from comment #48)
> > > +nsOpusState::nsOpusState(ogg_page* aBosPage) :
> > 
> > opus.h is only included if MOZ_OPUS is defined, so this won't compile
> > without MOZ_OPUS. As per the comment I made in nsOggCodecState.h, just wrap
> > the definition of nsOpusState's member functions in #ifdef MOZ_OPUS (unless
> > you can think of a better way).
> 
> Did you mean to comment out the bodies of the definitions? These functions
> are called from several places in nsOggReader, so I either need to provide
> dummy definitions, or add a *lot* of #ifdef'd code.

I meant wrap the definintions (in the header file) of nsOpusCodecState's members in #ifdef MOZ_OPUS, so if MOZ_OPUS isn't defined, no members are defined. Then when nsOggReader calls mOpusState's member functions, since nsOpusCodecState doesn't override nsOggCodecState's (non pure) virtual functions, callers will just call nsOggCodecState's functions instead. e.g.:

class nsOpusCodecState : public nsOggCodecState {
#ifdef MOZ_OPS
/* class body. */
#endif
};

> What about removing the MOZ_OPUS conditional entirely? We have a bunch of
> these in the media code, and I don't think it's worth the hassle of
> maintaining them.

Dunno. The media libraries have them so we only have to change a build switch in order to not be shipping the library's code, in case someone tries to assert a patent against us. Downstream users of our code may also appreciate them. Leave them in for now.


> >> nsOpusState::Reset
> >
> > I'm not sure it makes sense to return NS_ERROR_FAILURE if !mActive, the
> > other codec states don't do this.
> 
> You're right. It doesn't look like it's necessary. Does it make sense to
> still check this before actually resetting the codec? nsVorbisState::Reset
> does this as a side effect of using mActive as a proxy for initialization.

I think you should still reset the codec in this case.

> > if (aPacket->bytes < 16) {
> >   mActive = false;
> >   return true;
> 
> Thanks. What do 'true' and 'false' mean here? Do we need to return true on
> failure because we want to fail silently?

From http://mxr.mozilla.org/mozilla-central/source/content/media/ogg/nsOggCodecState.h#116
"Returns true when last header has been read."
If the packet's invalid, you don't need to read any more headers right?

> Would you like me to add a reject for the invalid case?

Yes please.


> You mean 'if (NS_FAILED(rv) || !haveGranulepos)' ?

Yes, thanks.

> Would 'if !(NS_SUCCEEDED(rv) || haveGranulepos)" be better?

"if (condition)" seems clearer than "if !(condition)" to me.

> It will be less confused by clamping to zero than by a -1?
> 
> I was thinking about preventing overflow here, but this may actually
> conflict with the trim code in nsOggReader::DecodeOpus. But if they have to
> be non-negative, clamping to zero isn't worse than any of the other failure
> modes. Any file where this occurs is technically invalid.

Ok. If this only occurs on invalid files, perhaps you should warn in this case?

In general we should do something sensible when we encounter invalid files, as we're likely to hit them at web scale.

Particularly once people start encoding files with ffmpeg.
Thanks, Ted, for the review. Here's an updated build patch which hopefully addresses your comments.

>> +ifdef MOZ_OPUS_FIXED
>
> Does this actually get defined anywhere, or is it just for testing purposes?

It's not defined anywhere, it's just a hook the build machinery can use to build the library with fixed-point rather than floating-point math. It should probably be defined for ARM builds, but I'd like to do that in a separate bug.
Attachment #614511 - Attachment is obsolete: true
Attachment #618365 - Flags: review?(ted.mielczarek)
Attachment #614511 - Flags: review?(tterribe)
Attached patch Add opus to nsOggReader (obsolete) — Splinter Review
Updated patch.

This one compiles with --disable-opus, but I'm stuck on what to do about the mochitest. test_buffered.html, when run in the test framework, times out because the opus test file can't be decoded. This test starts a bunch of video elements playing, and listens for the 'ended' event before checking some things about the buffered and duration attributes. Of course that event never arrives.

This seems to not be a problem for --disable-webm because no nsWebMReader is ever instantiated, while in the opus case we have to instantiate an nsOggReader to find out it can't decode Opus. Somehow those two error conditions are handled differently by the test framework.

I'm not sure what to do here. Some ideas:

We could propagate CPP symbols into the mochitest with a SpecialPowers object and add the Opus test file conditionally. That's Yet Another List to maintain, so I'd rather not.

We could listen for 'onerror' in test_buffered.html and handle the decode failure. It's tricky to tell what's an expected vs unexpected failure though. Maybe we can compare the filename extension against the presence of media.opus.enabled or the output of canPlayType.

Or, maybe there's some way to make nsOggReader fail more like --disable-webm, so that the object doesn't hang around not playing and cause a timeout.


Other responses inline:

> It'd kinda be nice to push the decode specific stuff down into the codec
> states, but it's not really worth the hassle at this stage I'd say.

I agree. That would be a separate bug anyway.

> > >> nsOpusState::Reset
> >
> > Does it make sense to still check this before actually resetting
> the codec? nsVorbisState::Reset does this as a side effect of using
> mActive as a proxy for initialization.
>
> I think you should still reset the codec in this case.

Sorry, you think I should reset the decoder only if mActive is true? Or regardless of its value? This patch does the former.

> If the packet's invalid, you don't need to read any more headers right?

Right.

>> Would you like me to add a reject for the invalid case?
>
> Yes please.

Added.

>> [clamping negative granulepos]
> Ok. If this only occurs on invalid files, perhaps you should warn
> in this case?

I've added debug messages for the various invalid header cases, and warn if we're clamping negative granulepos to zero. They're set to zero without a warning if opus_packet_get_nb_frames returns an error, which is only possible if the packet is truncated to less than two bytes.
Attachment #616270 - Attachment is obsolete: true
Attachment #618376 - Flags: review?(cpearce)
Comment on attachment 618365 [details] [diff] [review]
Add opus to the build system

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

For future reference, if I give you an r+ with comments I'm generally ok with you making the requested changes without requesting re-review. This looks fine.
Attachment #618365 - Flags: review?(ted.mielczarek) → review+
(In reply to Ted Mielczarek [:ted] from comment #62)

> For future reference, if I give you an r+ with comments I'm generally ok
> with you making the requested changes without requesting re-review. This
> looks fine.

Ok, thanks. I wasn't sure if patches needed an explicity r+ from the reviewer to land.
(In reply to Ralph Giles (:rillian) from comment #63)
> Ok, thanks. I wasn't sure if patches needed an explicity r+ from the
> reviewer to land.

Common practice is to say, "carrying forward r=whoever" when posting the patch, and set the r+ flag yourself (it'll show you as the reviewer in bugzilla, but that's not important... presumably you'll have the correct reviewer in the commit message).
Yay workflow documentation! And they say people use bugzilla in disjoint ways.
Comment on attachment 616869 [details] [diff] [review]
Add a short opus file to mochitest

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

::: content/media/test/manifest.js
@@ +8,5 @@
>    { name:"small-shot.ogg", type:"audio/ogg", duration:0.276 },
>    { name:"r11025_s16_c1.wav", type:"audio/x-wav", duration:1.0 },
>    { name:"320x240.ogv", type:"video/ogg", width:320, height:240, duration:0.233 },
>    { name:"seek.webm", type:"video/webm", duration:3.966 },
> +  { name:"detodos.opus", type:"audio/ogg", duration:2.9135 },

If you change 'type' to type:"audio/ogg; codecs=opus", does that fix the test timeout in test_buffered?
(In reply to Chris Pearce (:cpearce) from comment #66)

> If you change 'type' to type:"audio/ogg; codecs=opus", does that fix the
> test timeout in test_buffered?

That works! Very clever shortcut.

I still need to trap the error from setting 'media.opus.enabled' without MOZ_OPUS and disable the canPlayType tests, then we should be good.
Comment on attachment 618376 [details] [diff] [review]
Add opus to nsOggReader

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

r=cpearce with two minor changes. No need to re-request review.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1920,5 @@
>    if (IsOggType(nsDependentCString(aMIMEType))) {
>      *aCodecList = gOggCodecs;
> +#ifdef MOZ_OPUS
> +    if (IsOpusEnabled())
> +      *aCodecList = IsOpusEnabled() ? gOggCodecsWithOpus : gOggCodecs;

I meant:

if (IsOggType(nsDependentCString(aMIMEType))) {
  *aCodecList = IsOpusEnabled() ? gOggCodecsWithOpus : gOggCodecs;
  return CANPLAY_MAYBE;
}

::: content/media/ogg/nsOggReader.cpp
@@ +228,5 @@
>        if (codecState &&
> +          codecState->GetType() == nsOggCodecState::TYPE_OPUS &&
> +          !mOpusState)
> +      {
> +        if (mOpusEnabled)

{} around every if block's body unless it's a trivial error return please.
Attachment #618376 - Flags: review?(cpearce) → review+
(In reply to Timothy B. Terriberry (:derf) from comment #64)
> Common practice is to say, "carrying forward r=whoever" when posting the
> patch, and set the r+ flag yourself (it'll show you as the reviewer in
> bugzilla, but that's not important... presumably you'll have the correct
> reviewer in the commit message).

I don't recommend setting the flag yourself. It doesn't serve any purpose except to make it more confusing to find the actual reviewer.
Attached patch Add opus to nsOggReader (obsolete) — Splinter Review
Updated decoder patch to fix the final nits. Carrying forward r+ from :cpearce.
Attachment #618376 - Attachment is obsolete: true
Updated mochitest patch.

Two changes, which prevent failures when MOZ_OPUS isn't defined:

- Include 'codecs=opus' in the media type for the test file, which prevents timeouts.
- Guards the canPlayType tests against media.opus.enabled being undefined, preventing an unexpected failure.
Attachment #616869 - Attachment is obsolete: true
Try push of the latest patch set: https://tbpl.mozilla.org/?tree=Try&rev=cc83bc4471e6
We missed the window for ff14.
Target Milestone: mozilla14 → mozilla15
Thanks to derf for pushing the first two patches to inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/32e001c1351b

This bug should be kept open after those land in m-c. The decoder patches aren't ready to land yet; I still see inconsistent duration results from test_buffered.html.
Whiteboard: [mentor=rillian] [lang=c++] → [mentor=rillian] [lang=c++] [Leave open after merge]
Attachment #614508 - Flags: checkin+
Attachment #618365 - Flags: checkin+
After this landed, we're seeing some Windows PGO build bustages because the linker is running out of address space.  I suspect that this is the cause since it's the largest piece of code which has landed recently.  Can this code go into libgkmedias and not libxul?
(In reply to Ehsan Akhgari [:ehsan] from comment #75)
> Can this code go into libgkmedias and not libxul?

It *is* is libgkmedias.
Yes, the new code is in gkmedias.dll; confirmed with the win32-debug try build from comment #72.
Updated decoder patch. Carrying forward earlier r+ from Chris Pearce.

This contains two one-line changes, and fixes the intermittent new orange the previous version showed on try builds. See https://tbpl.mozilla.org/?tree=Try&rev=b536f7d5d03b; which shows no new test failures.

The fixes are from bug 746577 and bug 746891.
Attachment #618828 - Attachment is obsolete: true
Adding a new copy of the mochitest patch to keep the attachment ordering clear. No change from the previous patch, r+ by Chris Pearce.
Attachment #618837 - Attachment is obsolete: true
Ready to land when the tree re-opens. The first two patches are already on inbound. Please land the second two.
Keywords: checkin-needed
Whiteboard: [mentor=rillian] [lang=c++] [Leave open after merge] → [mentor=rillian] [lang=c++]
Blocks: 751219
Attachment #620337 - Flags: checkin+
Attachment #620338 - Flags: checkin+
(In reply to Eric Shepherd [:sheppy] from comment #84)

> https://developer.mozilla.org/En/
> Media_formats_supported_by_the_audio_and_video_elements#Ogg_Opus

Thanks, Eric!
No longer blocks: 748144
Depends on: 772341
Blocks: 928258
You need to log in before you can comment on or make changes to this bug.