736400 - media.[ogg,webm,wave].enabled cannot toggle off media playback

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris DeCairos (:cade)]

I noticed that if I toggle off any of these prefs, the media will still be able to playback in a webpage.

It does stop the media from playing back directly by navigating to the media src.

Steps to Reproduce:

1. Start Nightly (with pref enabled)
2. Play an ogg/webm video (audio types are probably similaryly affected, cannot confirm as of yet.)
3. in about:config, set media.ogg/webm.enabled to false
4. Reload page with video.

Expected result: type unsupported should be displayed
Actual result: Video is playable!

Comment 1

[Chris DeCairos (:cade)]

I'm going to take this, as it affects my work in bug 665395

Comment 2

[Chris DeCairos (:cade)]

Attachment: Makes Clone() methods check if its mimetype is enabled

Whats causing this is nsHTMLMediaElement::InitializeDecoderAsClone() which will call the decoder's clone method, which simply creates a new Decoder, without checking the pref for that type of media. 

I'm not sure this is the most elegant solution or if we should do the checking in InitializeDecoderAsClone() itself. The reason I did it in each decoder type separately was because the current mimetype of the decoder isn't available from within when the decoder is cloned (as far as I know). If it is available in some way, I can do the check in InitializeDecoderAsClone().

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 606501 [details] [diff] [review]
Makes Clone() methods check if its mimetype is enabled

Review of attachment 606501 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/ogg/nsOggDecoder.h
@@ +44,5 @@
>  class nsOggDecoder : public nsBuiltinDecoder
>  {
>  public:
> +  virtual nsMediaDecoder* Clone() {
> +    if ( !nsHTMLMediaElement::IsOggEnabled() ) {

*Always* use the prevaling style of the file/module you're adding code to.

i.e.:
1. No spaces between parenthesis and what's to its containing.
3. Use nsnull rather than 0 as a null pointer (since its easier for automated tools to disambiguate).
2. We use C++ style casts (since they're easier for automated tools to disambiguate), so that would be a static_cast<nsMediaDecoder*>(nsnull).
4. I don't think you need to cast nsnull to nsMediaDecoder* ?

So that should be:

if (!nsHTMLMediaElement::IsOggEnabled()) {
  return nsnull;
}

Same comments applies to all the Clone() methods.

Comment 4

[Chris DeCairos (:cade)]

Attachment: Fixed up as per review

Fixed up the spacing, and have the clone methods returning nsnull if the media type is disabled.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 6

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9f417cb5cd16 for Android build bustage along the lines of https://tbpl.mozilla.org/php/getParsedLog.php?id=10360607&tree=Mozilla-Inbound

Comment 7

[Chris DeCairos (:cade)]

Attachment: Made isRawEnabled() a static member of nsHTMLMediaElement

This patch makes isRawEnabled() a static method of nsHTMLMediaElement

I made sure to wrap it in "#ifdef MOZ_RAW" for good measure.

I *think* this should solve the android build issues. Unfortunately my account on hg.mozilla.org isn't working yet so I can't push to try and find out.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 609178 [details] [diff] [review]
Made isRawEnabled() a static member of nsHTMLMediaElement

Review of attachment 609178 [details] [diff] [review]:
-----------------------------------------------------------------

r=cpearce with the change below.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +277,5 @@
>    // false here even if CanHandleMediaType would return true.
>    static bool ShouldHandleMediaType(const char* aMIMEType);
>  
> +#ifdef MOZ_RAW
> +  static bool isRawEnabled();

IsRawEnabled (capital 'I').

Comment 9

[Chris DeCairos (:cade)]

Attachment: Fixed up capitalization of IsRawEnabled()

This built just fine on my machine with "ac_add_options --enable-raw" in my mozconfig.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad6c040729f

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Backed out, still broken on android xul.
https://hg.mozilla.org/integration/mozilla-inbound/rev/95c4ed9b56f2

Comment 12

[Chris DeCairos (:cade)]

Attachment: adds nsHTMLMediaElement:: to call for IsRawEnabled()

Alright, I believe I got all of the possible issues with Raw audio related methods. Still compiles on my machine. hopefully works on Android.

Comment 13

[Chris DeCairos (:cade)]

https://tbpl.mozilla.org/?tree=Try&rev=805e9ac9c7f8

Not sure if any of the problems in the tests are due to my patch. But I don't think they are.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/391418d235a8

Chris, please include your email address with your Hg user info for future patches.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/391418d235a8