938686 - Support Opus in WebM

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Jan Gerber]

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

Comment 1

[Jan Gerber]

Attachment: 0002-Refactor-Opus-header-parsing-so-it-can-be-reused.patch

Comment 2

[Jan Gerber]

Attachment: 0003-OggReader-DownmixToStereo-into-a-public-static-funct.patch

Comment 3

[Jan Gerber]

Attachment: 0004-Add-Opus-support-in-WebM.patch

Comment 4

[Ralph Giles (:rillian)]

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.

Comment 5

[Ralph Giles (:rillian)]

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.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 832351 [details] [diff] [review]
0004-Add-Opus-support-in-WebM.patch

Passing the Matthew for additional review.

Comment 7

[Ralph Giles (:rillian)]

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?

Comment 8

[Matthew Gregan [:kinetik]]

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.

Comment 9

[Alex Chronopoulos [:achronop]]

(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

[Alex Chronopoulos [:achronop]]

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?

Comment 11

[Jan Gerber]

(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

Comment 12

[Jan Gerber]

Attachment: 0001-Refactor-Opus-header-parsing-so-it-can-be-reused.patch

Comment 13

[Jan Gerber]

Attachment: 0003-Add-Opus-support-in-WebM.patch

Comment 14

[Matthew Gregan [:kinetik]]

(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.

Comment 15

[Jan Gerber]

Attachment: 0001-Refactor-Opus-header-parsing-so-it-can-be-reused.patch

make mParser nsAutoPtr

Comment 16

[Jan Gerber]

Attachment: 0003-Add-Opus-support-in-WebM.patch

Make mOpusParser nsAutoPtr and cleanup mOpusDecoder in line with OggReader

Comment 17

[Matthew Gregan [: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.

Comment 18

[Matthew Gregan [:kinetik]]

Er, I mean, you fixed that in the other patch. :-)

Comment 19

[Ralph Giles (:rillian)]

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

Comment 20

[Ralph Giles (:rillian)]

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

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b09ed61ac3fd

Next time, you should really push to try before landing...

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Daniel Veditz [:dveditz]]

We need to make sure this gets some fuzz testing

Comment 24

[Ralph Giles (:rillian)]

(In reply to Daniel Veditz [:dveditz] from comment #23)
> We need to make sure this gets some fuzz testing

Yes, please!

Comment 25

[Florian Bender]

(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?

Comment 26

[Jan Gerber]

(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.

Comment 27

[Florian Bender]

At least https://developer.mozilla.org/de/docs/HTML/Supported_media_formats needs to be updated before dev-doc-complete.

Comment 28

[Andrei Vaida [:avaida]]

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]