| Summary: | [GStreamer] Video player sets system volume to 100% | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jonathon Jongsma (jonner) <jonathon> | ||||||||||
| Component: | WebKitGTK | Assignee: | Xabier Rodríguez Calvar <calvaris> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Major | CC: | allan.jensen, calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gustavo, jer.noble, menard, mrobinson, patrakov, pnormand | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| URL: | http://www.w3.org/2010/05/video/mediaevents.html | ||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=145609 https://bugs.webkit.org/show_bug.cgi?id=140358 |
||||||||||||
| Bug Depends on: | 54140 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Jonathon Jongsma (jonner)
2013-07-22 10:30:58 PDT
+Xabier in CC, who's been looking at this issue too. Xabier, are you actively working on this, or would you like me to look into it? (In reply to comment #2) > Xabier, are you actively working on this, or would you like me to look into it? Not now. You can have a look if you want. (In reply to comment #3) > (In reply to comment #2) > > Xabier, are you actively working on this, or would you like me to look into it? > > Not now. You can have a look if you want. Anyway, if I have free time I could try to give it a look too, but I cannot guarantee anything. Created attachment 208081 [details]
Patch
(In reply to comment #5) > Created an attachment (id=208081) [details] > Patch This is a proof of concept, that's why I didn't request the review flag. This is a workaround I could think of. The ideas behind it is: * Keeping the latest volume used. * We could just avoid setting the volume when creating the MediaPlayer, but if we reset it like this, the behavior is more coherent when using another sink, like alsa. * In this case, both pulse with and without flat volumes work the same, the volume is kept from the last used. In the case of alsa, it always begins with 100%, but as it has relative volume to system, it shouldn't be a big deal. Alternatives to this workaround: * Another workaround of checking if we are using flat volume, which would require GStreamer exposing such property. * Using a volume element that would make us control the whole volume ourselves but we would lose audio passthough. Any thoughts? Comment on attachment 208081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208081&action=review Do the media tests still pass with that patch? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:61 > +static float lastVolume = -1.0; Perhaps name this gSavedVolumeValue or so... (In reply to comment #7) > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:61 > > +static float lastVolume = -1.0; > > Perhaps name this gSavedVolumeValue or so... This should presumably be a class-level property rather than a file-scope static variable, right? Because we want to ignore the very first volume set for *every* player that's created, not simply for the very first time we set volume on any media player. (In reply to comment #7) > (From update of attachment 208081 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208081&action=review > > Do the media tests still pass with that patch? Still have to do it. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:61 > > +static float lastVolume = -1.0; > > Perhaps name this gSavedVolumeValue or so... Roger that. (In reply to comment #8) > This should presumably be a class-level property rather than a file-scope static variable, right? Because we want to ignore the very first volume set for *every* player that's created, not simply for the very first time we set volume on any media player. For class level, do you mean static? I guess so because for non-static I understand object-level. If it's static you get the same behavior that in this case, and if it is not, you always avoid setting the volume for every mplayer that is created, but that's what I tried to prevent, otherwise, I would just ignore the volumes. The idea of keeping them like this is sharing them among the different media players. The problem with making a class property is that it's used in a callback so I'd need to either create a public method to reset it or make the attribute public. I would be ok with any solution regarding this specific part. (In reply to comment #10) > For class level, do you mean static? I guess so because for non-static I understand object-level. Sorry, ignore this comment. I meant a normal class member (non-static) but I misunderstood the patch slightly when I made that comment. This approach has some issues though. Consider the following case. - You play a video in webkitgtk, the lastVolume variable gets set to e.g. 0.3, which is your current system volume. - Now you browse to a page that has no video player. - While on this page, you change your system volume to 0.6 - Now you go to another page with a video player. When the media player is created, it will set the stream volume for that player to 0.3, because that's our last saved value - I suspect that 99% of people would expect the volume of that player to be 0.6 instead of 0.3 (In reply to comment #11) > This approach has some issues though. Consider the following case. > - You play a video in webkitgtk, the lastVolume variable gets set to e.g. 0.3, which is your current system volume. > - Now you browse to a page that has no video player. > - While on this page, you change your system volume to 0.6 > - Now you go to another page with a video player. When the media player is created, it will set the stream volume for that player to 0.3, because that's our last saved value > - I suspect that 99% of people would expect the volume of that player to be 0.6 instead of 0.3 Yes, that can be indeed a problem of this patch, though I don't know exactly if that could be the expected behavior. From my understanding, W3C standard (I cannot find where I had read it now), says that the default volume is 1, though the user agent can decide to cache the volume. From that you can understand that it is the cached volume or the new one you have set when changing the "system" volume . I write it with " because it's tricky to speak about system volume when dealing with flat volumes and also, from my little understanding of pulse flat volumes, you cannot say that the "system" volume you raise will be the one that you have set from the general volume slider. IIRC it can be either the mean or the max of those volumes, but this is based on my fragile memory so handle with care. Another thing we could do is just avoid setting the volume and trusting the sink, whatever it is. We would remove the nasty global variable (or class or object member) and it would behave reasonably well in every situation. In alsa it would raise to 100% of the stream volume, but it is relative to system one, which is under control and in the case of pulsesink, it would be handled by pulse, so it would select the latest volume, regardless of flat or non-flat volumes. (In reply to comment #12) > Another thing we could do is just avoid setting the volume and trusting the sink, whatever it is. We would remove the nasty global variable (or class or object member) and it would behave reasonably well in every situation. In alsa it would raise to 100% of the stream volume, but it is relative to system one, which is under control and in the case of pulsesink, it would be handled by pulse, so it would select the latest volume, regardless of flat or non-flat volumes. I personally think that this is probably a better approach and will generally give more expected behavior in most situations. This only applies to setting the volume the first time, of course. After the first one, we should set the volume as requested. That still makes it possible for people to get blasted if a page changes the volume to 1.0 via javascript or something, but that's unavoidable unless we do a much more complicated implementation. Created attachment 208208 [details]
Patch
(In reply to comment #14) > Created an attachment (id=208208) [details] > Patch I reworked the patch in order not to set the volume when creating the pipeline. This breaks media/video-volume.html that expects the volume to be 1.0 when the media player is created. I see three ways of facing this problem: * Removing the first tests, checking for 1.0 from the test and changing all expectations * Setting the volume to 1 at the beginning of the test * Flagging the test (in all affected ports) * Creating expectations about that (in all affected ports) (In reply to comment #15) > This breaks media/video-volume.html that expects the volume to be 1.0 when the media player is created. I see three ways of facing this problem: It looks like media/video-volume.html test revealed some interesting issues about my patch proposal and it looks like you can be changing the volume of a MediaPlayer prior to having a GStreamer pipeline so somebody could be changing the MediaPlayer volume and we wouldn't be acknowledging the change after the media was loaded, so we need to think of something else. (In reply to comment #6) > This is a proof of concept, that's why I didn't request the review flag. > > This is a workaround I could think of. The ideas behind it is: > > * Keeping the latest volume used. > * We could just avoid setting the volume when creating the MediaPlayer, but if we reset it like this, the behavior is more coherent when using another sink, like alsa. > * In this case, both pulse with and without flat volumes work the same, the volume is kept from the last used. In the case of alsa, it always begins with 100%, but as it has relative volume to system, it shouldn't be a big deal. > > Alternatives to this workaround: > * Another workaround of checking if we are using flat volume, which would require GStreamer exposing such property. > * Using a volume element that would make us control the whole volume ourselves but we would lose audio passthough. > > Any thoughts? +1 to the second alternative, -1 to the main idea, and actually -1 to any attempt to control the mixer if flat volumes are enabled. IMHO, with flat volumes, the only sensible idea is to use a volume element that provides software-based relative volume control inside webkit (completely invisible to pulseaudio), and ignore (and don't touch) the volume on the sink. Otherwise you'll run into the following problem: * Suppose a desktop environment (e.g. Xfce) that has an OSD program that displays a popup showing the current volume if it changes for any reason. * Suppose a web page that wants to adjust the volume of its own stream * Due, to the change of the stream volume, pulseaudio (according to the flat-volume logic) will change the system volume * An OSD popup appears. That's bad. And also into this problem: * Suppose a web page that provides a user with a volume control, initially at 100%. * Suppose that a user changed that to 80%. * According to the flat volume logic, the system volume is also decreased, say, to 80% of what it was. * Suppose that a user changed the in-web-page volume to 70%. * Now webkit has to virtualize that by setting the stream volume to something. If it sets that to 70% of the initial volume, then the adjustments made between steps 1 and 3 outside of the webapp are effectively ignored. If it is set to 70% of the current system volume, that would be only 56% of the initial volume, i.e. too low. I.e. webkit has to notice that the app changed the volume down by 12.5%, read the current stream volume, and down that by 12.5%. And now we have a problem of special-casing 0%. * That's a rather complex and fragile approach, there will be a problem of understanding this by new developers, and they will break it someday. An explicit software volume element, controlled only by webkit and invisible to pulseaudio is so much easier :) Created attachment 209549 [details]
Patch
This is also a proof of concept; I guess something that we could do to improve this would be adding a compilation option to enable this feature (and/or a runtime option) and I also accept suggestions about the name of the variables and methods. The idea is leaving the responsibility of initializing the volume to the HTMLMediaElement. media/video-volume.html test is working now.
Comment on attachment 209549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209549&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:627 > + if (m_player->volumeWasInitialized()) { > + LOG_MEDIA_MESSAGE("Setting stream volume to %f", m_player->volume()); > + g_object_set(m_volumeElement.get(), "volume", m_player->volume(), NULL); I find this a bit confusing, perhaps because of the name of the new method. I'm not great at naming stuff but here's a suggestion: requiresPlatformVolumeConfiguration :) (In reply to comment #19) > (From update of attachment 209549 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209549&action=review > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:627 > > + if (m_player->volumeWasInitialized()) { > > + LOG_MEDIA_MESSAGE("Setting stream volume to %f", m_player->volume()); > > + g_object_set(m_volumeElement.get(), "volume", m_player->volume(), NULL); > > I find this a bit confusing, perhaps because of the name of the new method. I'm not great at naming stuff but here's a suggestion: requiresPlatformVolumeConfiguration :) Roger that. What about guarding the code with #IFDEFs? As it is now, it should work in all platforms because of the default behavior for the virtual method but maybe the Apple guys don't want that anyway. Phil, Eric, more opinions? Comment on attachment 209549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209549&action=review > Source/WebCore/platform/graphics/MediaPlayer.h:217 > + virtual bool mediaPlayerVolumeWasInitialized() const { return true; } Nit: It doesn't matter functionally because HTMLMediaElement overrides this virtual method, but shouldn't the default return be false? >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:627 >>> + g_object_set(m_volumeElement.get(), "volume", m_player->volume(), NULL); >> >> I find this a bit confusing, perhaps because of the name of the new method. I'm not great at naming stuff but here's a suggestion: requiresPlatformVolumeConfiguration :) > > Roger that. > > What about guarding the code with #IFDEFs? As it is now, it should work in all platforms because of the default behavior for the virtual method but maybe the Apple guys don't want that anyway. > > Phil, Eric, more opinions? I don't think it is worth adding another #if for this small bit of code. Created attachment 210212 [details]
Patch
Changed method name and added changelog.
Comment on attachment 210212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210212&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:627 > + if (!m_player->platformVolumeConfigurationRequired()) { > + LOG_MEDIA_MESSAGE("Setting stream volume to %f", m_player->volume()); > + g_object_set(m_volumeElement.get(), "volume", m_player->volume(), NULL); This still doesn't make sense to me. If no platform volume configuration is required we set the platform volume? (In reply to comment #23) > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:627 > > + if (!m_player->platformVolumeConfigurationRequired()) { > > + LOG_MEDIA_MESSAGE("Setting stream volume to %f", m_player->volume()); > > + g_object_set(m_volumeElement.get(), "volume", m_player->volume(), NULL); > > This still doesn't make sense to me. If no platform volume configuration is required we set the platform volume? Yes, because it it is required, we let the sink decide it for us. If it isn't because it was already reset in upper layers, then we set it to what the upper layer has. I am neutral to the patch if there is no way to change the kind of volume that the patch deals with from javascript. If there is such way, then I strongly object, on the basis that the volume may be not from user input and 100% would then mean the hardware maximum on some platforms, instead of the "default volume". (In reply to comment #25) > I am neutral to the patch if there is no way to change the kind of volume that the patch deals with from javascript. > > If there is such way, then I strongly object, on the basis that the volume may be not from user input and 100% would then mean the hardware maximum on some platforms, instead of the "default volume". Quoting the HTML5 spec: "The element's effective media volume is volume, interpreted relative to the range 0.0 to 1.0, with 0.0 being silent, and 1.0 being the loudest setting, values in between increasing in loudness. The range need not be linear. The loudest setting may be lower than the system's loudest possible setting; for example the user could have set a maximum volume." There's no way to change which type of volume is used from javascript because this is a platform-specific aspect that doesn't need to be exposed to the Web. (In reply to comment #26) > "The element's effective media volume is volume, interpreted relative to the range 0.0 to 1.0, with 0.0 being silent, and 1.0 being the loudest setting, values in between increasing in loudness. The range need not be linear. The loudest setting may be lower than the system's loudest possible setting; for example the user could have set a maximum volume." > > There's no way to change which type of volume is used from javascript because this is a platform-specific aspect that doesn't need to be exposed to the Web. Sorry for wording my comment in such a way that you misinterpreted it. I meant: if there is a way to set the media volume from javascript to a floating-point value (such as 0.99) not based on the user input, then I am strongly against the patch, as it doesn't solve the problem of sudden too-loud sounds at all. The reason is a combination of security and common sense. Even though the spec says that the loudest setting MAY be lower than the system's loudest possible setting, configurable volume limiting is in fact a required feature for protection against malicious javascript. In Windows, this is done by interpreting the volume as being relative to the system mixer - i.e. there is no way for the web page to sound louder than allowed by the system mixer. And I think that there are web pages that rely on such behaviour and assume that setting the volume to 1.0 is safe for the user's hearing. On systems such as pulseaudio with flat volumes this assumption is not true. So you need not only to prevent the initial setting of the volume to 1.0, but also to modify the values that application sets as volumes. And as soon as you start doing that, you are no longer doing a volume passthrough. In fact the whole point of this comment is that it is wrong to do a passthrough if the volume that the application attempts to set may be not based on the user input. I suggest never touching the stream volume in gstreamer, on any platform, as this is provided for user-initiated changes only. Instead, please insert an explicit volume element into the gstreamer pipeline and use it for web media volume changes. Then, even on flat-volume pulseaudio platform, there is no way for a web page to sound louder than the system mixer initially allows. Comment on attachment 210212 [details] Patch Clearing flags on attachment: 210212 Committed r154970: <http://trac.webkit.org/changeset/154970> All reviewed patches have been landed. Closing bug. Not fixed. See http://jsfiddle.net/bteam/FbkGD/ (suggested by my colleague) for a testcase that stays on close-to-maximum system volume on PulseAudio with flat volumes. (In reply to comment #30) > Not fixed. See http://jsfiddle.net/bteam/FbkGD/ (suggested by my colleague) for a testcase that stays on close-to-maximum system volume on PulseAudio with flat volumes. As I can see in that web the volume is intentionally set by the website to 0.99 and 1.0 so we consider that a bug in the webpage. We need to keep WebKit's behavior consistent with the rest of desktop applications using pulseaudio, meaning that if as a consequence of rising the app volume, the system volume is risen too, we shall do the same, even taking into account "buggy" websites. If you want that behavior changed, you should change your flat volume settings in pulseaudio. (In reply to comment #31) > (In reply to comment #30) > > Not fixed. See http://jsfiddle.net/bteam/FbkGD/ (suggested by my colleague) for a testcase that stays on close-to-maximum system volume on PulseAudio with flat volumes. > > As I can see in that web the volume is intentionally set by the website to 0.99 and 1.0 so we consider that a bug in the webpage. Sorry, I have to disagree here. It is not a bug in the webpage. This webpage is malicious by design. It is a security issue in webkit under Linux/pulseaudio that it obeys to such malicious volume requests literally. The whole security model of pulseaudio is that applications will not make such malicious requests, and it is unreasonable to expect that from web pages directly. So some barrier needs to be there between web application volume requests and pulseaudio. In Windows, or without flat volumes, the security model is that the application-set volume applies as a percentage of the system mixer volume, and thus the 1.0 volume is safe, and even matches the default. I think it would be a big stretch to demand the knowledge that such flat volume environments exist from Windows-only web developers. They just assume that 1.0 means "whatever the system mixer says". > We need to keep WebKit's behavior consistent with the rest of desktop applications using pulseaudio, meaning that if as a consequence of rising the app volume, the system volume is risen too, we shall do the same, even taking into account "buggy" websites. If you want that behavior changed, you should change your flat volume settings in pulseaudio. Two points here. 1. I will take this to pulseaudio developers again. I completely agree that flat volumes are a huge bug in their product, but it is also the default that we have to deal with, for now. If you want to influence that, please come to the audio miniconf at LinuxCon Europe. I will be there, too. 2. Given that flat volumes exist, I disagree with the phrase "We need to keep WebKit's behavior consistent with the rest of desktop applications using pulseaudio", because this would be impossible without the above-mentioned security issue. (In reply to comment #32) > 1. I will take this to pulseaudio developers again. I completely agree that flat volumes are a huge bug in their product, but it is also the default that we have to deal with, for now. If you want to influence that, please come to the audio miniconf at LinuxCon Europe. I will be there, too. Good, come back then and tell us. Thanks! After thinking a bit more about this, I changed my mind.
Now my opinion is that using the volume on the sink (and not a volume element) is wrong even in the non-flat-volume case. In other words, attempting to squeeze the web content into a model of "behavior consistent with the rest of desktop applications" is an inherently flawed idea. The reason is the same: malicious web pages that change the volume of their audio players without user input.
The same test page (trivially amended to also unmute itself) will resist to attempts to make its volume lower than the rest of the applications, or to mute it, even in non-flat-volume case. In non-flat-volume case, though, I would consider it as just a bug ("does not obey the mixer"), not as a security issue.
One more consideration is consistency with Adobe Flash and Lightspark players. Flash, by necessity, uses relative volumes (equivalent to the volume element), and Lightspark (which was also affected by a similar bug some time ago) now properly virtualizes the volume changes made by Flash content, using the same relative model inconsistent with desktop applications. In my opinion, consistency with Flash is more important here than consistency with desktop applications.
The pulseaudio flat model is broken and stupid by design. It should not be used. Could GStreamer perhaps detect bad pulseaudio configuration and add a relative volume control so that playbin and volum-element has consistant behavior? This change broke the ability of an application to set a volume multiplier (Page::setMediaVolume). See https://bugs.webkit.org/show_bug.cgi?id=137305. |