Support Opus in WebM

VERIFIED FIXED in mozilla28

Status

()

Core
Audio/Video
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Jan Gerber, Assigned: Jan Gerber)

Tracking

({dev-doc-needed, feature})

Trunk
mozilla28
dev-doc-needed, feature
Points:
---
Dependency tree / graph
Bug Flags:
sec-review ?

Firefox Tracking Flags

(relnote-firefox 28+)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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

4 years ago
Depends on: 938315
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 1

4 years ago
Created attachment 832349 [details] [diff] [review]
0002-Refactor-Opus-header-parsing-so-it-can-be-reused.patch
(Assignee)

Comment 2

4 years ago
Created attachment 832350 [details] [diff] [review]
0003-OggReader-DownmixToStereo-into-a-public-static-funct.patch
(Assignee)

Comment 3

4 years ago
Created attachment 832351 [details] [diff] [review]
0004-Add-Opus-support-in-WebM.patch
(Assignee)

Updated

4 years ago
Attachment #832349 - Flags: review?(giles)
(Assignee)

Updated

4 years ago
Attachment #832350 - Flags: review?(giles)
(Assignee)

Updated

4 years ago
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?
(Assignee)

Comment 11

4 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

4 years ago
Created attachment 8336810 [details] [diff] [review]
0001-Refactor-Opus-header-parsing-so-it-can-be-reused.patch
Attachment #832349 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8336811 [details] [diff] [review]
0003-Add-Opus-support-in-WebM.patch
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.
(Assignee)

Comment 15

4 years ago
Created attachment 8337063 [details] [diff] [review]
0001-Refactor-Opus-header-parsing-so-it-can-be-reused.patch

make mParser nsAutoPtr
Attachment #8336810 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Created attachment 8337064 [details] [diff] [review]
0003-Add-Opus-support-in-WebM.patch

Make mOpusParser nsAutoPtr and cleanup mOpusDecoder in line with OggReader
Attachment #8336811 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8337063 - Flags: review?(kinetik)
(Assignee)

Updated

4 years ago
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+
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
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...
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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!

Comment 25

4 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

4 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)

Updated

4 years ago
Blocks: 946180
(Assignee)

Updated

4 years ago
Blocks: 945863

Comment 27

4 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
relnote-firefox: ? → 28+

Updated

4 years ago
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
You need to log in before you can comment on or make changes to this bug.