Last Comment Bug 736400 - media.[ogg,webm,wave].enabled cannot toggle off media playback
: media.[ogg,webm,wave].enabled cannot toggle off media playback
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Chris DeCairos (:cade)
:
Mentors:
Depends on:
Blocks: 665395
  Show dependency treegraph
 
Reported: 2012-03-16 00:21 PDT by Chris DeCairos (:cade)
Modified: 2012-03-31 19:25 PDT (History)
3 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Makes Clone() methods check if its mimetype is enabled (2.80 KB, patch)
2012-03-16 02:05 PDT, Chris DeCairos (:cade)
cpearce: review-
Details | Diff | Splinter Review
Fixed up as per review (2.75 KB, patch)
2012-03-18 16:33 PDT, Chris DeCairos (:cade)
cpearce: review+
Details | Diff | Splinter Review
Made isRawEnabled() a static member of nsHTMLMediaElement (4.30 KB, patch)
2012-03-25 17:37 PDT, Chris DeCairos (:cade)
cpearce: review+
Details | Diff | Splinter Review
Fixed up capitalization of IsRawEnabled() (4.30 KB, patch)
2012-03-25 18:16 PDT, Chris DeCairos (:cade)
cpearce: review+
Details | Diff | Splinter Review
adds nsHTMLMediaElement:: to call for IsRawEnabled() (4.49 KB, patch)
2012-03-25 19:08 PDT, Chris DeCairos (:cade)
cpearce: review+
Details | Diff | Splinter Review

Description Chris DeCairos (:cade) 2012-03-16 00:21:21 PDT
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) 2012-03-16 00:22:11 PDT
I'm going to take this, as it affects my work in bug 665395
Comment 2 Chris DeCairos (:cade) 2012-03-16 02:05:43 PDT
Created attachment 606501 [details] [diff] [review]
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 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-18 14:27:05 PDT
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) 2012-03-18 16:33:59 PDT
Created attachment 607031 [details] [diff] [review]
Fixed up as per review

Fixed up the spacing, and have the clone methods returning nsnull if the media type is disabled.
Comment 5 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-25 14:51:58 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a4b0171b99
Comment 6 Phil Ringnalda (:philor) 2012-03-25 15:39:23 PDT
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) 2012-03-25 17:37:03 PDT
Created attachment 609178 [details] [diff] [review]
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 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-25 17:57:59 PDT
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) 2012-03-25 18:16:42 PDT
Created attachment 609182 [details] [diff] [review]
Fixed up capitalization of IsRawEnabled()

This built just fine on my machine with "ac_add_options --enable-raw" in my mozconfig.
Comment 10 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-25 18:24:36 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad6c040729f
Comment 11 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-25 18:42:52 PDT
Backed out, still broken on android xul.
https://hg.mozilla.org/integration/mozilla-inbound/rev/95c4ed9b56f2
Comment 12 Chris DeCairos (:cade) 2012-03-25 19:08:14 PDT
Created attachment 609187 [details] [diff] [review]
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) 2012-03-26 09:22:33 PDT
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] 2012-03-30 18:00:55 PDT
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] 2012-03-31 19:25:44 PDT
https://hg.mozilla.org/mozilla-central/rev/391418d235a8

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