Closed Bug 736400 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: cade, Assigned: cade)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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!
I'm going to take this, as it affects my work in bug 665395
Status: NEW → ASSIGNED
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().
Attachment #606501 - Flags: review?(cpearce)
Blocks: 665395
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.
Attachment #606501 - Flags: review?(cpearce) → review-
Attached patch Fixed up as per review (obsolete) — Splinter Review
Fixed up the spacing, and have the clone methods returning nsnull if the media type is disabled.
Attachment #606501 - Attachment is obsolete: true
Attachment #607031 - Flags: review?(cpearce)
Attachment #607031 - Flags: review?(cpearce) → review+
Keywords: checkin-needed
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
Assignee: nobody → chris
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: mozilla14 → ---
Version: unspecified → Trunk
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.
Attachment #607031 - Attachment is obsolete: true
Attachment #609178 - Flags: review?(cpearce)
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').
Attachment #609178 - Flags: review?(cpearce) → review+
This built just fine on my machine with "ac_add_options --enable-raw" in my mozconfig.
Attachment #609178 - Attachment is obsolete: true
Attachment #609182 - Flags: review?(cpearce)
Attachment #609182 - Flags: review?(cpearce) → review+
Alright, I believe I got all of the possible issues with Raw audio related methods. Still compiles on my machine. hopefully works on Android.
Attachment #609182 - Attachment is obsolete: true
Attachment #609187 - Flags: review?(cpearce)
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.
Attachment #609187 - Flags: review?(cpearce) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/391418d235a8

Chris, please include your email address with your Hg user info for future patches.
Flags: in-testsuite?
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/391418d235a8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.