Add default media volume setting

VERIFIED FIXED in Firefox 49

Status

()

enhancement
P2
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: ibragimovrinat, Unassigned)

Tracking

46 Branch
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

3 years ago
Posted patch default-media-volume.patch (obsolete) — Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160508001106

Steps to reproduce:

Open a page with <video> tag in it.


Actual results:

Volume setting is at 100%.


Expected results:

Volume is at user-configured level, specified by media.default_volume setting (on the about:config page)
Reporter

Updated

3 years ago
Severity: normal → enhancement
OS: Unspecified → All
Hardware: Unspecified → All
Reporter

Updated

3 years ago
Attachment #8754899 - Attachment is obsolete: true
Reporter

Comment 1

3 years ago
rebased on top of current state of the Firefox source
Reporter

Updated

3 years ago
Component: Untriaged → Audio/Video
Product: Firefox → Core
Reporter

Updated

3 years ago
Attachment #8754909 - Flags: review?(kinetik)
Component: Audio/Video → Audio/Video: Playback
Comment on attachment 8754909 [details] [diff] [review]
default-media-volume-hg-tip.patch

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

Patch is fine with GetCString -> GetFloat fixed, but there's a general move towards removing media prefs going on at the moment, so I'm not sure we want to add this one.  We could possibly add this and remove media.volume_scale (which is less generally useful).

::: dom/html/HTMLMediaElement.cpp
@@ +92,5 @@
>  #include "nsRange.h"
>  #include <algorithm>
>  #include <cmath>
>  
> +#include "prdtoa.h"

No need to include this; see below.

@@ +2278,5 @@
>      mFirstFrameLoaded(false),
>      mDefaultPlaybackStartPosition(0.0),
>      mIsAudioTrackAudible(false)
>  {
> +  nsAdoptingCString defVolumeString = Preferences::GetCString("media.default_volume");

Use Preferences::GetFloat() instead.
Attachment #8754909 - Flags: review?(kinetik)
ni? for opinion on adding this pref.
Flags: needinfo?(ajones)
Reporter

Updated

3 years ago
Attachment #8754909 - Attachment is obsolete: true
Reporter

Comment 4

3 years ago
Posted patch default-media-volume-v2.patch (obsolete) — Splinter Review
add media.default_volume setting for initial media volume. v2.
Reporter

Updated

3 years ago
Attachment #8755982 - Flags: review?(kinetik)
Reporter

Comment 5

3 years ago
Changed patch to use Preferences::GetFloat(), as suggested. Rebased on current hg tip.
(In reply to Matthew Gregan [:kinetik] from comment #3)
> ni? for opinion on adding this pref.

You could argue that an add-on could implement the same thing without needing to change gecko, however I'm OK with adding the pref.
Flags: needinfo?(ajones)
Comment on attachment 8755982 [details] [diff] [review]
default-media-volume-v2.patch

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

Looks good, thanks!
Attachment #8755982 - Flags: review?(kinetik) → review+
Reporter

Comment 8

3 years ago
(In reply to Matthew Gregan [:kinetik] from comment #7)
> Looks good, thanks!

Thank you.

I guess, the next step is to run patch through a "try server". I definitely have no access to any of that.

Could you help me with that, please?
(In reply to Rinat from comment #8)
> (In reply to Matthew Gregan [:kinetik] from comment #7)
> > Looks good, thanks!
> 
> Thank you.
> 
> I guess, the next step is to run patch through a "try server". I definitely
> have no access to any of that.
> 
> Could you help me with that, please?

No problem.  I pushed your patch to Try, you can follow the progress here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=226f8e593521

If you're interested in getting Try access for future patches, there's some information at https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server and https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/#Level1, and feel free to ping me on IRC or email if you need any help.
Reporter

Comment 10

3 years ago
(In reply to Matthew Gregan [:kinetik] from comment #9)

> I pushed your patch to Try

Thanks!

> follow the progress here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=226f8e593521

A lot of red boxes telling me there was something wrong with the build. There were a number of patches before current, so it could be there were some interference. I think, it's better for me to apply for a Try Server access to push the changeset independently.
Yeah, looks like a bad base revision to use.  I pushed again with a better one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fb7f7853f6b
Reporter

Comment 12

3 years ago
(In reply to Matthew Gregan [:kinetik] from comment #11)
> Yeah, looks like a bad base revision to use.  I pushed again with a better
> one:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fb7f7853f6b

Thank you.

I think I need to nag you once more (sorry). This build has more green results, but there are still orange and red ones. But it looks like there are duplicated entries. The red bc7 result (timeout) have another bc7 next to it, which is green. All entries I tried have a link to a bug report with similar output.

Is that fine? Or should I do anything about it?
Posted patch rebasedSplinter Review
(In reply to Rinat from comment #12)
> I think I need to nag you once more (sorry). This build has more green
> results, but there are still orange and red ones. But it looks like there
> are duplicated entries. The red bc7 result (timeout) have another bc7 next
> to it, which is green. All entries I tried have a link to a bug report with
> similar output.
> 
> Is that fine? Or should I do anything about it?


It looks like it's fine, the failures are intermittent (where there are green retries) or they're not near the code you've changed.

The patch doesn't apply any longer, so I've rebased it and it's ready for landing.  I'd land it directly, but the tree is currently closed.
Attachment #8755982 - Attachment is obsolete: true
Attachment #8759024 - Flags: review+

Comment 14

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94c374f96f84
Add media.default_volume preference.  r=kinetik
Keywords: checkin-needed
Reporter

Comment 15

3 years ago
(In reply to Matthew Gregan [:kinetik] from comment #13)


Thanks!

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/94c374f96f84
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Reporter

Comment 17

3 years ago
Just tried a Nightly build. The media.default_volume setting works as expected.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.