Closed
Bug 938686
Opened 11 years ago
Closed 11 years ago
Support Opus in WebM
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
VERIFIED
FIXED
mozilla28
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 28+ |
People
(Reporter: j, Assigned: j)
References
Details
(Keywords: dev-doc-needed, feature)
Attachments
(3 files, 4 obsolete files)
1.74 KB,
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
18.76 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
16.25 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release) Build ID: 20131113030205 Steps to reproduce: Together with VP9 Opus should be supported in WebM
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #832349 -
Flags: review?(giles)
Assignee | ||
Updated•11 years ago
|
Attachment #832350 -
Flags: review?(giles)
Assignee | ||
Updated•11 years ago
|
Attachment #832351 -
Flags: review?(giles)
Comment 4•11 years ago
|
||
Comment on attachment 832351 [details] [diff] [review] 0004-Add-Opus-support-in-WebM.patch Review of attachment 832351 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me. The Opus decoder is of course duplicate code with the one in OggReader, but I'm ok with that now; please open a bug to merge them. I think it would be easier to read if DecodeAudioPacket called out to DecodeVorbisPacket and DecodeOpusPacket instead of the giant conditional. ::: content/media/webm/WebMReader.cpp @@ +724,3 @@ > mAudioFrames += frames; > + > +#endif /* MOZ_OPUS */ Should return false here in an #else clause.
Attachment #832351 -
Flags: review?(giles) → review+
Comment 5•11 years ago
|
||
Comment on attachment 832350 [details] [diff] [review] 0003-OggReader-DownmixToStereo-into-a-public-static-funct.patch Review of attachment 832350 [details] [diff] [review]: ----------------------------------------------------------------- r=me This will bitrot bug 911482, btw.
Attachment #832350 -
Flags: review?(giles) → review+
Comment 6•11 years ago
|
||
Comment on attachment 832351 [details] [diff] [review] 0004-Add-Opus-support-in-WebM.patch Passing the Matthew for additional review.
Attachment #832351 -
Flags: review?(kinetik)
Comment 7•11 years ago
|
||
Comment on attachment 832349 [details] [diff] [review] 0002-Refactor-Opus-header-parsing-so-it-can-be-reused.patch Review of attachment 832349 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed. ::: content/media/ogg/OpusParser.cpp @@ +24,5 @@ > +namespace mozilla { > + > +#ifdef PR_LOGGING > +extern PRLogModuleInfo* gMediaDecoderLog; > +#define LOG(type, msg) PR_LOG(gMediaDecoderLog, type, msg) Please update this for Bug 939582 changes. ::: content/media/ogg/OpusParser.h @@ +12,5 @@ > +#include "opus/opus_multistream.h" > +// For MOZ_SAMPLE_TYPE_* > +#include "mozilla/dom/HTMLMediaElement.h" > +#include "MediaDecoderStateMachine.h" > +#include "MediaDecoderReader.h" Do you still need these headers?
Attachment #832349 -
Flags: review?(giles) → review+
Comment 8•11 years ago
|
||
Comment on attachment 832351 [details] [diff] [review] 0004-Add-Opus-support-in-WebM.patch Review of attachment 832351 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webm/WebMReader.cpp @@ +422,2 @@ > > + if(!InitOpusDecoder()) { Space between if and ( here and above. @@ +447,5 @@ > return NS_OK; > } > > +#ifdef MOZ_OPUS > +bool WebMReader::InitOpusDecoder(void) No need for void in C++. @@ +449,5 @@ > > +#ifdef MOZ_OPUS > +bool WebMReader::InitOpusDecoder(void) > +{ > + int error; s/error/r/ ::: content/media/webm/WebMReader.h @@ +159,5 @@ > int64_t aGranulepos); > > +#ifdef MOZ_OPUS > + // Setup opus decoder > + bool InitOpusDecoder(void); No void.
Attachment #832351 -
Flags: review?(kinetik) → review+
Comment 9•11 years ago
|
||
(In reply to Ralph Giles (:rillian) from comment #5) > > This will bitrot bug 911482, btw. If this patch is landed before mine I will take care of this. Otherwise Jan should update his patch. Thanks!
Comment 10•11 years ago
|
||
Comment on attachment 832351 [details] [diff] [review] 0004-Add-Opus-support-in-WebM.patch Review of attachment 832351 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webm/WebMReader.cpp @@ +413,5 @@ > Cleanup(); > return NS_ERROR_FAILURE; > } > > + mOpusParser = new OpusParser; I do not see mOpusParser deleted anywhere. Is it a potential problem or I am missing something?
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Ralph Giles (:rillian) from comment #7) > ::: content/media/ogg/OpusParser.cpp > @@ +24,5 @@ > > +namespace mozilla { > > + > > +#ifdef PR_LOGGING > > +extern PRLogModuleInfo* gMediaDecoderLog; > > +#define LOG(type, msg) PR_LOG(gMediaDecoderLog, type, msg) > > Please update this for Bug 939582 changes. changed to LOG_OPUS > ::: content/media/ogg/OpusParser.h > @@ +12,5 @@ > > +#include "opus/opus_multistream.h" > > +// For MOZ_SAMPLE_TYPE_* > > +#include "mozilla/dom/HTMLMediaElement.h" > > +#include "MediaDecoderStateMachine.h" > > +#include "MediaDecoderReader.h" > > Do you still need these headers? removed (In reply to Ralph Giles (:rillian) from comment #4) > ::: content/media/webm/WebMReader.cpp > @@ +724,3 @@ > > mAudioFrames += frames; > > + > > +#endif /* MOZ_OPUS */ > > Should return false here in an #else clause. ok (In reply to Matthew Gregan [:kinetik] from comment #8) > ::: content/media/webm/WebMReader.cpp > @@ +422,2 @@ > > > > + if(!InitOpusDecoder()) { > > Space between if and ( here and above. ok > > @@ +447,5 @@ > > return NS_OK; > > } > > > > +#ifdef MOZ_OPUS > > +bool WebMReader::InitOpusDecoder(void) > > No need for void in C++. ok > @@ +449,5 @@ > > > > +#ifdef MOZ_OPUS > > +bool WebMReader::InitOpusDecoder(void) > > +{ > > + int error; > > s/error/r/ ok > ::: content/media/webm/WebMReader.h > @@ +159,5 @@ > > int64_t aGranulepos); > > > > +#ifdef MOZ_OPUS > > + // Setup opus decoder > > + bool InitOpusDecoder(void); > > No void. ok
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #832349 -
Attachment is obsolete: true
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #832351 -
Attachment is obsolete: true
Comment 14•11 years ago
|
||
(In reply to Alexandros Chronopoulos [:achronop] from comment #10) > I do not see mOpusParser deleted anywhere. Is it a potential problem or I am > missing something? Er... yeah. mOpusParser and mOpusDecoder should be nsAutoPtr/nsAutoRefs.
Assignee | ||
Comment 15•11 years ago
|
||
make mParser nsAutoPtr
Attachment #8336810 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
Make mOpusParser nsAutoPtr and cleanup mOpusDecoder in line with OggReader
Attachment #8336811 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8337063 -
Flags: review?(kinetik)
Assignee | ||
Updated•11 years ago
|
Attachment #8337064 -
Flags: review?(kinetik)
Comment 17•11 years ago
|
||
Comment on attachment 8337063 [details] [diff] [review] 0001-Refactor-Opus-header-parsing-so-it-can-be-reused.patch Review of attachment 8337063 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. I mentioned mDecoder too, but I hadn't checked the code. mDecoder is freed in the dtor, so that's fine already.
Attachment #8337063 -
Flags: review?(kinetik) → review+
Comment 18•11 years ago
|
||
Er, I mean, you fixed that in the other patch. :-)
Updated•11 years ago
|
Attachment #8337064 -
Flags: review?(kinetik) → review+
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1cca0891dbe https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2dbfc2463f https://hg.mozilla.org/integration/mozilla-inbound/rev/0575a10042e2
Assignee: nobody → j
Keywords: dev-doc-needed
Comment 20•11 years ago
|
||
Hotfix -Werror. https://hg.mozilla.org/integration/mozilla-inbound/rev/e014d47fc576 Glandium also found with a non-unified build: "content/media/ogg/OpusParser.cpp:70:16: error: use of undeclared identifier 'LEUint16' haha, and it's a static function in a completely different cpp file" Which we need to fix.
Comment 21•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09ed61ac3fd Next time, you should really push to try before landing...
Comment 22•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f1cca0891dbe https://hg.mozilla.org/mozilla-central/rev/8c2dbfc2463f https://hg.mozilla.org/mozilla-central/rev/0575a10042e2 https://hg.mozilla.org/mozilla-central/rev/e014d47fc576 https://hg.mozilla.org/mozilla-central/rev/b09ed61ac3fd
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment 23•11 years ago
|
||
We need to make sure this gets some fuzz testing
Flags: sec-review?(cdiehl)
Comment 24•11 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #23) > We need to make sure this gets some fuzz testing Yes, please!
Comment 25•11 years ago
|
||
(In reply to Ralph Giles (:rillian) from comment #4) > Looks fine to me. The Opus decoder is of course duplicate code with the one > in OggReader, but I'm ok with that now; please open a bug to merge them. Did this happen yet?
Flags: needinfo?(j)
Assignee | ||
Comment 26•11 years ago
|
||
(In reply to Florian Bender from comment #25) > (In reply to Ralph Giles (:rillian) from comment #4) > > Looks fine to me. The Opus decoder is of course duplicate code with the one > > in OggReader, but I'm ok with that now; please open a bug to merge them. > > Did this happen yet? Opened Bug 946180 for that.
Flags: needinfo?(j)
Comment 27•11 years ago
|
||
At least https://developer.mozilla.org/de/docs/HTML/Supported_media_formats needs to be updated before dev-doc-complete.
relnote-firefox:
--- → ?
Keywords: feature
Updated•10 years ago
|
Comment 28•10 years ago
|
||
User agents: [1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 [2] Mozilla/5.0 (Windows NT 6.3; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 [3] Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 [4] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 I was able to confirm the fix for this issue on the latest Beta (Build ID: 20140213172947), using the samples attached to Bug 945513 with: - Windows 7 64-bit [1] - Windows 8.1 Pro 64-bit [2] - Ubuntu 13.10 64-bit [3] - Mac OS X 10.9 [4]
Status: RESOLVED → VERIFIED
Keywords: verifyme
Updated•10 years ago
|
QA Contact: andrei.vaida
Updated•6 years ago
|
Flags: sec-review?(cdiehl)
You need to log in
before you can comment on or make changes to this bug.
Description
•