Last Comment Bug 938686 - Support Opus in WebM
: Support Opus in WebM
Status: VERIFIED FIXED
: dev-doc-needed, feature
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla28
Assigned To: Jan Gerber
: Andrei Vaida, QA [:avaida] – please ni? me
: Maire Reavy [:mreavy] Please needinfo me
Mentors:
Depends on: 938315 945532 1066943
Blocks: 945513 945863 946180
  Show dependency treegraph
 
Reported: 2013-11-14 10:14 PST by Jan Gerber
Modified: 2014-09-17 16:43 PDT (History)
12 users (show)
dveditz: sec‑review? (cdiehl)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
28+


Attachments
0002-Refactor-Opus-header-parsing-so-it-can-be-reused.patch (19.74 KB, patch)
2013-11-14 10:16 PST, Jan Gerber
giles: review+
Details | Diff | Splinter Review
0003-OggReader-DownmixToStereo-into-a-public-static-funct.patch (1.74 KB, patch)
2013-11-14 10:16 PST, Jan Gerber
giles: review+
Details | Diff | Splinter Review
0004-Add-Opus-support-in-WebM.patch (16.15 KB, patch)
2013-11-14 10:17 PST, Jan Gerber
giles: review+
kinetik: review+
Details | Diff | Splinter Review
0001-Refactor-Opus-header-parsing-so-it-can-be-reused.patch (18.75 KB, patch)
2013-11-22 07:25 PST, Jan Gerber
no flags Details | Diff | Splinter Review
0003-Add-Opus-support-in-WebM.patch (16.04 KB, patch)
2013-11-22 07:25 PST, Jan Gerber
no flags Details | Diff | Splinter Review
0001-Refactor-Opus-header-parsing-so-it-can-be-reused.patch (18.76 KB, patch)
2013-11-22 14:06 PST, Jan Gerber
kinetik: review+
Details | Diff | Splinter Review
0003-Add-Opus-support-in-WebM.patch (16.25 KB, patch)
2013-11-22 14:07 PST, Jan Gerber
kinetik: review+
Details | Diff | Splinter Review

Description User image Jan Gerber 2013-11-14 10:14:11 PST
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 User image Jan Gerber 2013-11-14 10:16:21 PST
Created attachment 832349 [details] [diff] [review]
0002-Refactor-Opus-header-parsing-so-it-can-be-reused.patch
Comment 2 User image Jan Gerber 2013-11-14 10:16:44 PST
Created attachment 832350 [details] [diff] [review]
0003-OggReader-DownmixToStereo-into-a-public-static-funct.patch
Comment 3 User image Jan Gerber 2013-11-14 10:17:02 PST
Created attachment 832351 [details] [diff] [review]
0004-Add-Opus-support-in-WebM.patch
Comment 4 User image Ralph Giles (:rillian) | needinfo me 2013-11-21 14:36:23 PST
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 User image Ralph Giles (:rillian) | needinfo me 2013-11-21 14:38:08 PST
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 User image Ralph Giles (:rillian) | needinfo me 2013-11-21 14:39:12 PST
Comment on attachment 832351 [details] [diff] [review]
0004-Add-Opus-support-in-WebM.patch

Passing the Matthew for additional review.
Comment 7 User image Ralph Giles (:rillian) | needinfo me 2013-11-21 14:52:14 PST
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 User image Matthew Gregan [:kinetik] 2013-11-21 16:28:24 PST
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 User image Alex Chronopoulos [:achronop] 2013-11-21 23:38:55 PST
(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 User image Alex Chronopoulos [:achronop] 2013-11-21 23:55:08 PST
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 User image Jan Gerber 2013-11-22 07:24:36 PST
(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 User image Jan Gerber 2013-11-22 07:25:21 PST
Created attachment 8336810 [details] [diff] [review]
0001-Refactor-Opus-header-parsing-so-it-can-be-reused.patch
Comment 13 User image Jan Gerber 2013-11-22 07:25:54 PST
Created attachment 8336811 [details] [diff] [review]
0003-Add-Opus-support-in-WebM.patch
Comment 14 User image Matthew Gregan [:kinetik] 2013-11-22 12:09:13 PST
(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 User image Jan Gerber 2013-11-22 14:06:57 PST
Created attachment 8337063 [details] [diff] [review]
0001-Refactor-Opus-header-parsing-so-it-can-be-reused.patch

make mParser nsAutoPtr
Comment 16 User image Jan Gerber 2013-11-22 14:07:53 PST
Created attachment 8337064 [details] [diff] [review]
0003-Add-Opus-support-in-WebM.patch

Make mOpusParser nsAutoPtr and cleanup mOpusDecoder in line with OggReader
Comment 17 User image Matthew Gregan [:kinetik] 2013-11-26 14:00:40 PST
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 User image Matthew Gregan [:kinetik] 2013-11-26 14:02:26 PST
Er, I mean, you fixed that in the other patch. :-)
Comment 20 User image Ralph Giles (:rillian) | needinfo me 2013-12-02 16:27:39 PST
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 User image Mike Hommey [:glandium] 2013-12-02 17:54:09 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09ed61ac3fd

Next time, you should really push to try before landing...
Comment 23 User image Daniel Veditz [:dveditz] 2013-12-03 11:53:48 PST
We need to make sure this gets some fuzz testing
Comment 24 User image Ralph Giles (:rillian) | needinfo me 2013-12-03 12:35:58 PST
(In reply to Daniel Veditz [:dveditz] from comment #23)
> We need to make sure this gets some fuzz testing

Yes, please!
Comment 25 User image Florian Bender 2013-12-04 03:15:09 PST
(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 User image Jan Gerber 2013-12-04 03:55:45 PST
(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 User image Florian Bender 2013-12-06 14:52:21 PST
At least https://developer.mozilla.org/de/docs/HTML/Supported_media_formats needs to be updated before dev-doc-complete.
Comment 28 User image Andrei Vaida, QA [:avaida] – please ni? me 2014-02-17 23:29:02 PST
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]

Note You need to log in before you can comment on or make changes to this bug.