Closed Bug 938686 Opened 11 years ago Closed 10 years ago

Support Opus in WebM

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

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)

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
Depends on: 938315
OS: Linux → All
Hardware: x86_64 → All
Attachment #832349 - Flags: review?(giles)
Attachment #832350 - Flags: review?(giles)
Attachment #832351 - Flags: review?(giles)
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 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 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 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 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+
(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 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?
(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
Attachment #832349 - Attachment is obsolete: true
Attachment #832351 - Attachment is obsolete: true
(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.
make mParser nsAutoPtr
Attachment #8336810 - Attachment is obsolete: true
Make mOpusParser nsAutoPtr and cleanup mOpusDecoder in line with OggReader
Attachment #8336811 - Attachment is obsolete: true
Attachment #8337063 - Flags: review?(kinetik)
Attachment #8337064 - Flags: review?(kinetik)
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+
Er, I mean, you fixed that in the other patch. :-)
Attachment #8337064 - Flags: review?(kinetik) → review+
Blocks: 945513
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.
Depends on: 945532
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09ed61ac3fd

Next time, you should really push to try before landing...
We need to make sure this gets some fuzz testing
Flags: sec-review?(cdiehl)
(In reply to Daniel Veditz [:dveditz] from comment #23)
> We need to make sure this gets some fuzz testing

Yes, please!
(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)
(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)
Blocks: 946180
Blocks: 945863
At least https://developer.mozilla.org/de/docs/HTML/Supported_media_formats needs to be updated before dev-doc-complete.
relnote-firefox: --- → ?
Keywords: feature
Keywords: verifyme
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
QA Contact: andrei.vaida
Depends on: 1066943
Flags: sec-review?(cdiehl)
You need to log in before you can comment on or make changes to this bug.