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

RESOLVED FIXED in mozilla14

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cade, Assigned: cade)

Tracking

(Blocks: 1 bug)

Trunk
mozilla14
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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!
(Assignee)

Comment 1

5 years ago
I'm going to take this, as it affects my work in bug 665395
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
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().
Attachment #606501 - Flags: review?(cpearce)
(Assignee)

Updated

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

Comment 4

5 years ago
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.
Attachment #606501 - Attachment is obsolete: true
Attachment #607031 - Flags: review?(cpearce)
Attachment #607031 - Flags: review?(cpearce) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a4b0171b99
Keywords: checkin-needed
Target Milestone: --- → mozilla14
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
(Assignee)

Comment 7

5 years ago
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.
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+
(Assignee)

Comment 9

5 years ago
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.
Attachment #609178 - Attachment is obsolete: true
Attachment #609182 - Flags: review?(cpearce)
Attachment #609182 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad6c040729f
Backed out, still broken on android xul.
https://hg.mozilla.org/integration/mozilla-inbound/rev/95c4ed9b56f2
(Assignee)

Comment 12

5 years ago
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.
Attachment #609182 - Attachment is obsolete: true
Attachment #609187 - Flags: review?(cpearce)
(Assignee)

Comment 13

5 years ago
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+
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.