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)
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/
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 1•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
Assignee | ||
Comment 3•13 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Whiteboard: [mentor=rillian] [lang=c++]
Assignee | ||
Comment 4•12 years ago
|
||
Update to the latest IETF draft.
Attachment #578448 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
Some updates from old work.
Attachment #578450 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Attachment #610223 -
Attachment is patch: true
Assignee | ||
Comment 7•12 years ago
|
||
Status update: The current plan is to land this pref'd off in firefox 14.
Target Milestone: --- → mozilla14
Version: 5 Branch → Trunk
Comment 8•12 years ago
|
||
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.
Updated•12 years ago
|
Assignee: nobody → giles
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
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
Assignee | ||
Comment 12•12 years ago
|
||
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)
Assignee | ||
Comment 13•12 years ago
|
||
New patch disabling decode support by default.
Attachment #611660 -
Flags: review?(cpearce)
Assignee | ||
Updated•12 years ago
|
Attachment #607664 -
Flags: review?(tterribe)
Attachment #607664 -
Flags: review?(cpearce)
Assignee | ||
Updated•12 years ago
|
Attachment #607641 -
Flags: review?(tterribe)
Comment 14•12 years ago
|
||
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 15•12 years ago
|
||
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 16•12 years ago
|
||
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 17•12 years ago
|
||
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 18•12 years ago
|
||
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 19•12 years ago
|
||
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 20•12 years ago
|
||
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-
Assignee | ||
Comment 21•12 years ago
|
||
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
Assignee | ||
Comment 22•12 years ago
|
||
Assignee | ||
Comment 23•12 years ago
|
||
'git hgp' didn't include the binary test file in the previous attachment. This goes is content/media/test/ along with opus-test.patch.
Assignee | ||
Comment 24•12 years ago
|
||
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
Assignee | ||
Comment 25•12 years ago
|
||
Update the build system patch in response to Tim's commments.
Attachment #607664 -
Attachment is obsolete: true
Assignee | ||
Comment 26•12 years ago
|
||
Update the decoder patch for the opus.h namespace change in the most recent build patch.
Attachment #611963 -
Attachment is obsolete: true
Assignee | ||
Comment 27•12 years ago
|
||
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.
Comment 28•12 years ago
|
||
(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...
Assignee | ||
Comment 29•12 years ago
|
||
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.
Comment 30•12 years ago
|
||
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
Comment 31•12 years ago
|
||
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.
Comment 32•12 years ago
|
||
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.
Assignee | ||
Comment 33•12 years ago
|
||
Attachment #614508 -
Flags: review?(tterribe)
Assignee | ||
Comment 34•12 years ago
|
||
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
Assignee | ||
Comment 35•12 years ago
|
||
Updated build patch. This fixes issues on MSVC and Android.
Attachment #613398 -
Attachment is obsolete: true
Assignee | ||
Comment 36•12 years ago
|
||
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
Assignee | ||
Comment 37•12 years ago
|
||
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
Assignee | ||
Comment 38•12 years ago
|
||
New patch set pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=dbf42684fe7c
Comment 39•12 years ago
|
||
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+
Assignee | ||
Comment 40•12 years ago
|
||
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.
Comment 41•12 years ago
|
||
(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.
Assignee | ||
Comment 42•12 years ago
|
||
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)
Assignee | ||
Comment 43•12 years ago
|
||
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.
Assignee | ||
Comment 44•12 years ago
|
||
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
Assignee | ||
Comment 45•12 years ago
|
||
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
Assignee | ||
Comment 46•12 years ago
|
||
Green try run at https://tbpl.mozilla.org/?tree=Try&rev=a229203a80f4
Assignee | ||
Updated•12 years ago
|
Attachment #616270 -
Flags: review?(cpearce)
Assignee | ||
Updated•12 years ago
|
Attachment #616278 -
Flags: review?(cpearce)
Assignee | ||
Comment 47•12 years ago
|
||
The intermittant 'leak running tests' on WinXP debug seems to be bug 723832.
Comment 48•12 years ago
|
||
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 49•12 years ago
|
||
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 50•12 years ago
|
||
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.
Comment 51•12 years ago
|
||
(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).
Comment 52•12 years ago
|
||
(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.
Comment 53•12 years ago
|
||
Ok, just leave it as an assertion then.
Assignee | ||
Comment 54•12 years ago
|
||
> 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.
Assignee | ||
Comment 55•12 years ago
|
||
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 56•12 years ago
|
||
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+
Assignee | ||
Comment 57•12 years ago
|
||
(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.
Assignee | ||
Comment 58•12 years ago
|
||
> 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.
Comment 59•12 years ago
|
||
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.
Assignee | ||
Comment 60•12 years ago
|
||
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)
Assignee | ||
Comment 61•12 years ago
|
||
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 62•12 years ago
|
||
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+
Assignee | ||
Comment 63•12 years ago
|
||
(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.
Comment 64•12 years ago
|
||
(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).
Assignee | ||
Comment 65•12 years ago
|
||
Yay workflow documentation! And they say people use bugzilla in disjoint ways.
Comment 66•12 years ago
|
||
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?
Assignee | ||
Comment 67•12 years ago
|
||
(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 68•12 years ago
|
||
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+
Comment 69•12 years ago
|
||
(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.
Assignee | ||
Comment 70•12 years ago
|
||
Updated decoder patch to fix the final nits. Carrying forward r+ from :cpearce.
Attachment #618376 -
Attachment is obsolete: true
Assignee | ||
Comment 71•12 years ago
|
||
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
Assignee | ||
Comment 72•12 years ago
|
||
Try push of the latest patch set: https://tbpl.mozilla.org/?tree=Try&rev=cc83bc4471e6
Assignee | ||
Comment 73•12 years ago
|
||
We missed the window for ff14.
Target Milestone: mozilla14 → mozilla15
Assignee | ||
Comment 74•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [mentor=rillian] [lang=c++] → [mentor=rillian] [lang=c++] [Leave open after merge]
Assignee | ||
Updated•12 years ago
|
Attachment #614508 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #618365 -
Flags: checkin+
Comment 75•12 years ago
|
||
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?
Comment 76•12 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #75) > Can this code go into libgkmedias and not libxul? It *is* is libgkmedias.
Assignee | ||
Comment 77•12 years ago
|
||
Yes, the new code is in gkmedias.dll; confirmed with the win32-debug try build from comment #72.
Assignee | ||
Comment 78•12 years ago
|
||
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
Assignee | ||
Comment 79•12 years ago
|
||
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
Assignee | ||
Comment 80•12 years ago
|
||
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++]
Comment 81•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8bb81a28639c https://hg.mozilla.org/mozilla-central/rev/32e001c1351b
Comment 82•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b150579861e https://hg.mozilla.org/integration/mozilla-inbound/rev/d71d3b331f4e
Flags: in-testsuite+
Keywords: checkin-needed
Updated•12 years ago
|
Attachment #620337 -
Flags: checkin+
Updated•12 years ago
|
Attachment #620338 -
Flags: checkin+
Comment 83•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3b150579861e https://hg.mozilla.org/mozilla-central/rev/d71d3b331f4e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 84•12 years ago
|
||
Docs updated: https://developer.mozilla.org/En/Media_formats_supported_by_the_audio_and_video_elements#Ogg_Opus mentioned on Firefox 15 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 85•12 years ago
|
||
(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!
You need to log in
before you can comment on or make changes to this bug.
Description
•