| Summary: | WKPageSetMediaVolume() API couldn't be used to restore the volume | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Vivek Arumugam <vivek_arumugam> | ||||||||
| Component: | WebKit API | Assignee: | Xabier Rodríguez Calvar <calvaris> | ||||||||
| Status: | NEW --- | ||||||||||
| Severity: | Normal | CC: | calvaris, eric.carlson, ews-watchlist, jer.noble, pvollan, ramasamy_thalavaypillai, rniwa, vivek_arumugam, youennf | ||||||||
| Priority: | P2 | ||||||||||
| Version: | Other | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
Attachment 339705 [details] did not pass style-queue:
ERROR: Source/WebCore/html/HTMLMediaElement.cpp:4819: Missing space before ( in if( [whitespace/parens] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Vivek Arumugam from comment #0) > The WKPageSetMediaVolume() API takes a floating point input ranging 0-1 > which is used as a multiplier against all the media elements of a page thus > acts as a master volume control. But however the current implementation > doesn't maintain the actual volume set from the upper layers (i.e JS / Media > Player App) so every time UI Process calls this API the internal volume gets > only reduced thus preventing us to get the previous volume back. Result is > this API can't be used to temporarily reduce and restore the volume on > demand. Yes, as we discussed in private, we'd need Dimm and UnDimm API that goes from the WebKit API layer down to the HTMLMediaElement. In the HTMLMediaElement or the MediaPlayer we would keep the former volume and when Dimming and restore it without multiplying when UnDimming. Introducing another set of APIs to reduce and restore the volume might be redundant and requires few assumptions as below: 1) If we are to have an API to Dim the volume do we need to pass any value / should we assume that it should reduce the volume to 25% or someother value? 2) And if we are to pass some value, again, we can't be passing any absolute value from UI Process because at that moment the media players (from different documents) from the same page might have different volumes. This argument supports the use of factored/multiplier approach as WKPageSetMediaVolume(). If we are worried about breaking existing functionality of WKPageSetMediaVolume(), we should first confirm that the purpose of WKPageSetMediaVolume() is different than what I think it is. (In reply to Vivek Arumugam from comment #3) > 1) If we are to have an API to Dim the volume do we need to pass any value / > should we assume that it should reduce the volume to 25% or someother value? IMHO, it should take a value. > 2) And if we are to pass some value, again, we can't be passing any absolute > value from UI Process because at that moment the media players (from > different documents) from the same page might have different volumes. This > argument supports the use of factored/multiplier approach as > WKPageSetMediaVolume(). This value would reduce the volume as it is done for WKPageSetMediaVolume but it would be recoverable after with UnDimm. > If we are worried about breaking existing functionality of > WKPageSetMediaVolume(), we should first confirm that the purpose of > WKPageSetMediaVolume() is different than what I think it is. We need to consider this, yes. If a set is made while the volume is dimmed, we need to deal with it somehow, either by failing or ignoring the set or by failing/ignoring the undimm. (In reply to Xabier Rodríguez Calvar from comment #4) > (In reply to Vivek Arumugam from comment #3) > > 2) And if we are to pass some value, again, we can't be passing any absolute > > value from UI Process because at that moment the media players (from > > different documents) from the same page might have different volumes. This > > argument supports the use of factored/multiplier approach as > > WKPageSetMediaVolume(). > > This value would reduce the volume as it is done for WKPageSetMediaVolume > but it would be recoverable after with UnDimm. > > > If we are worried about breaking existing functionality of > > WKPageSetMediaVolume(), we should first confirm that the purpose of > > WKPageSetMediaVolume() is different than what I think it is. > > We need to consider this, yes. If a set is made while the volume is dimmed, > we need to deal with it somehow, either by failing or ignoring the set or by > failing/ignoring the undimm. I would prefer to fix the existing API if possible instead of introducing a new one. The proposed change will not break any usage I am aware of. Comment on attachment 339705 [details] . View in context: https://bugs.webkit.org/attachment.cgi?id=339705&action=review > Source/WebCore/html/HTMLMediaElement.h:969 > + double m_refVolume { 1 }; There is no need to abbreviate: m_referenceVolume. Created attachment 339715 [details]
.
Addressed comments, please review.
Comment on attachment 339715 [details] . View in context: https://bugs.webkit.org/attachment.cgi?id=339715&action=review We can't land this as it does not have ChangeLog. > Source/WebCore/html/HTMLMediaElement.cpp:4820 > + Page* page = document().page(); > + if (!page || page->mediaVolume() == 1) > + m_referenceVolume = vol; I see a big problem with this code. Let's think of this use case: 1. m_volume = 1; m_referenceVolume = 1; page->m_mediaVolume = 1; 2. setPageMediaVolume(0.5); -> m_volume = 0.5; m_referenceVolume = 1; page->m_mediaVolume = 0.5; 3. Lower the stream volume to 0.25 -> m_volume = 0.25; m_referenceVolume = 1; page->m_mediaVolume = 0.5; 4. setPageMediaVolume(1); -> m_volume = 1; m_referenceVolume = 1; page->m_mediaVolume = 1; Step 4 is a problem, because you lowered the volume to almost nothing and then the set page volume ramps it up to 1 when the user had lowered the volume to 0.25. Eric, I think this behavior is wrong. We should consider, at least, m_referenceVolume to scale as if page volume were always 1 since we are already multiplying it with the volumeMultiplier when going back down to the player. If we go for this solution, we should, IMHO, instead of this code, do: m_referenceVolume = m_volume; if (page) m_referenceVolume /= page->mediaVolume(); And if we accept my premise, we could even consider m_referenceVolume to be redundant and consider that m_volume is always scaled with reference to the page being 1. If we consider this alternative, we might want to touch HTMLMediaElement::setReadyState and HTMLMediaElement::volume() to correct with the page volume or we would be getting lots of tests failing. We might just change HTMLMediaElement::volume() and have HTMLMediaElement::setReadyState using HTMLMediaElement::volume(). I would also add a couple of comments in m_volume and HTMLMediaElement::volume() regarding this. Created attachment 339945 [details]
Patch
I think this is a better solution for the issue we have here.
(In reply to Xabier Rodríguez Calvar from comment #8) > Comment on attachment 339715 [details] > . > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339715&action=review > > We can't land this as it does not have ChangeLog. > > > Source/WebCore/html/HTMLMediaElement.cpp:4820 > > + Page* page = document().page(); > > + if (!page || page->mediaVolume() == 1) > > + m_referenceVolume = vol; > > I see a big problem with this code. Let's think of this use case: > > 1. m_volume = 1; m_referenceVolume = 1; page->m_mediaVolume = 1; > 2. setPageMediaVolume(0.5); -> m_volume = 0.5; m_referenceVolume = 1; > page->m_mediaVolume = 0.5; > 3. Lower the stream volume to 0.25 -> m_volume = 0.25; m_referenceVolume = > 1; page->m_mediaVolume = 0.5; > 4. setPageMediaVolume(1); -> m_volume = 1; m_referenceVolume = 1; > page->m_mediaVolume = 1; > > Step 4 is a problem, because you lowered the volume to almost nothing and > then the set page volume ramps it up to 1 when the user had lowered the > volume to 0.25. > > Eric, I think this behavior is wrong. We should consider, at least, > m_referenceVolume to scale as if page volume were always 1 since we are > already multiplying it with the volumeMultiplier when going back down to the > player. If we go for this solution, we should, IMHO, instead of this code, > do: > m_referenceVolume = m_volume; > if (page) > m_referenceVolume /= page->mediaVolume(); > > And if we accept my premise, we could even consider m_referenceVolume to be > redundant and consider that m_volume is always scaled with reference to the > page being 1. If we consider this alternative, we might want to touch > HTMLMediaElement::setReadyState and HTMLMediaElement::volume() to correct > with the page volume or we would be getting lots of tests failing. We might > just change HTMLMediaElement::volume() and have > HTMLMediaElement::setReadyState using HTMLMediaElement::volume(). I would > also add a couple of comments in m_volume and HTMLMediaElement::volume() > regarding this. Hi Calvaris, Thank you for the review. I would like to know what do you mean by step 3? Which code flow is that? If you mean HTMLMediaElement::setVolume() the m_referenceVolume too gets updated with m_volume. Please clarify. (In reply to Vivek Arumugam from comment #10) > Hi Calvaris, Thank you for the review. I would like to know what do you mean > by step 3? Which code flow is that? If you mean > HTMLMediaElement::setVolume() the m_referenceVolume too gets updated with > m_volume. Please clarify. Volume can be updated through pulseaudio or other volume managers so we would still get callbacks for those changes which would end up in a situation like step 3. Comment on attachment 339945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339945&action=review > Source/WebCore/html/HTMLMediaElement.cpp:3634 > + Page* page = document().page(); Nit: auto* page = ... > Source/WebCore/html/HTMLMediaElement.cpp:3657 > + Page* page = document().page(); > + if (page) > + m_volume /= page->mediaVolume(); > + if (m_mediaController) > + m_volume /= m_mediaController->volume(); > +#if ENABLE(MEDIA_SESSION) > + if (m_shouldDuck) > + m_volume /= 0.25; > +#endif This is identical to the code in volume(), it should be shared. > Source/WebCore/html/HTMLMediaElement.cpp:3658 > + m_volume = CLAMP(m_volume, 0.0, 1.0); You can use clampToFloat() from MathExtras.h > Source/WebCore/html/HTMLMediaElement.cpp:3675 > + setScaledVolume(newVolume); setScaledVolume() will redo the same calculations just done in the call to volume() above. If setScaledVolume() returned a bool if the volume doesn't new volume is identical to the current volume, you could use it above and only do the calculations once. > Source/WebCore/html/HTMLMediaElement.cpp:4841 > + if (vol != volume()) { > + setScaledVolume(vol); Ditto. > Source/WebCore/html/HTMLMediaElement.cpp:5243 > + if (volume() != volume) { > + setScaledVolume(volume); Ditto. > Source/WebCore/html/HTMLMediaElement.cpp:5252 > + Page* page = document().page(); Nit: auto* page = ... > Source/WebCore/html/HTMLMediaElement.cpp:5257 > + m_player->setVolume(volume()); Why calculate the volume again, won't m_volume be correct after the call to setScaledVolume()? (In reply to Xabier Rodríguez Calvar from comment #8) > Comment on attachment 339715 [details] > > I see a big problem with this code. Let's think of this use case: > > 1. m_volume = 1; m_referenceVolume = 1; page->m_mediaVolume = 1; > 2. setPageMediaVolume(0.5); -> m_volume = 0.5; m_referenceVolume = 1; > page->m_mediaVolume = 0.5; > 3. Lower the stream volume to 0.25 -> m_volume = 0.25; m_referenceVolume = > 1; page->m_mediaVolume = 0.5; > 4. setPageMediaVolume(1); -> m_volume = 1; m_referenceVolume = 1; > page->m_mediaVolume = 1; > > Step 4 is a problem, because you lowered the volume to almost nothing and > then the set page volume ramps it up to 1 when the user had lowered the > volume to 0.25. > > Eric, I think this behavior is wrong. Good point, thanks for thinking this through! (In reply to Eric Carlson from comment #12) > > Source/WebCore/html/HTMLMediaElement.cpp:3634 > > + Page* page = document().page(); > > Nit: auto* page = ... Roger. > > Source/WebCore/html/HTMLMediaElement.cpp:3657 > > + Page* page = document().page(); > > + if (page) > > + m_volume /= page->mediaVolume(); > > + if (m_mediaController) > > + m_volume /= m_mediaController->volume(); > > +#if ENABLE(MEDIA_SESSION) > > + if (m_shouldDuck) > > + m_volume /= 0.25; > > +#endif > > This is identical to the code in volume(), it should be shared. No, one multiplies, the other divides. > > Source/WebCore/html/HTMLMediaElement.cpp:3658 > > + m_volume = CLAMP(m_volume, 0.0, 1.0); > > You can use clampToFloat() from MathExtras.h Roger. > > Source/WebCore/html/HTMLMediaElement.cpp:3675 > > + setScaledVolume(newVolume); > > setScaledVolume() will redo the same calculations just done in the call to > volume() above. If setScaledVolume() returned a bool if the volume doesn't > new volume is identical to the current volume, you could use it above and > only do the calculations once. I can work try to work this out. > > Source/WebCore/html/HTMLMediaElement.cpp:5257 > > + m_player->setVolume(volume()); > > Why calculate the volume again, won't m_volume be correct after the call to > setScaledVolume()? No, the idea was that in the HTMLMediaElement, we kept the volume unscaled from the Page and then, when committing to the player, we could apply the scale. Eric: Vivek and I had private conversations about the results of this patch and the truth is that it does not work either. Let's see: | Operation | Page | Element | Player | |--------------------------:|-----:|---------:|-------:| | Init | 1 | 1 | 1 | | Element::setVolume(0.7) | 1 | 0.7 | 0.7 | | Page::setMediaVolume(0.1) | 0.1 | 0.7 | 0.07 | | Element::setVolume(0.25) | 0.1 | 2.5 -> 1 | 1 | What I tried to do with the patch was trying to decouple the player volume and the element volume so that we could recover the player volume when resetting the page volume. The problem is that HTMLMediaElement::setVolume should respect the volume of the player (and so should setting it through any other API, like pulseaudio), which didn't happen with my patch leading to situations like the one here. If we consider the page multiplier as it was, we always go down when we change it and we can't recover from that. If we try to keep the different scales, we can get out of them and screw up. We could consider the multiplier the first time, keep the same scale between player and element but again we wouldn't be able to recover and the page scale would be out of sync. My opinion on what we should do to fix the current mess is consider the Page::setMediaVolume a one time only thing. The page decides to scale the volume? Fine, we do it and forget. That volume change would be translated to element and player but if JS, the element controls or external entities (of course I talk about non-IOS devices) decide to ramp up the volume, they could. Always respecting the page volume would mean that we'd have to reduce the scale in our volume controls, meaning that if page volume is set to 50%, volume would have to move between 0..50% and we'd have to find a way to deal with volumes higher than that set by JS or set by external volume controls. JS could be easier, we clamp to page volume and we can be happy, but external controls would be another issue because we can't set the scale there, you would drag that volume bar and see it coming down to the page volume, which would be weird. Then we still have the problem of Dimming and UnDimming the volume and the only solution I can think of to do this properly would be a Dimm and UnDimm API. Dimm would keep the page volume in the element and UnDimm would undo the dimming if the volume has not been touched by hand in any other control. |
Created attachment 339705 [details] . The WKPageSetMediaVolume() API takes a floating point input ranging 0-1 which is used as a multiplier against all the media elements of a page thus acts as a master volume control. But however the current implementation doesn't maintain the actual volume set from the upper layers (i.e JS / Media Player App) so every time UI Process calls this API the internal volume gets only reduced thus preventing us to get the previous volume back. Result is this API can't be used to temporarily reduce and restore the volume on demand. An example scenario is : * Assume initial volume of HTMLMediaElement is 0.9 & the multiplier (page::mediaVolume()) is 1 (Media player plays with full volume) * Do WKPageSetMediaVolume(xyz, 0.5) * page::mediaVolume() = 0.5 * HTMLMediaPlayer's m_volume is updated as (m_volume = 0.9 * page->mediaVolume = 0.5) = 0.45 * Do WKPageSetMediaVolume(xyz, 1) * page::mediaVolume() = 1 * HTMLMediaPlayer's m_volume is updated as (m_volume = 0.45 * page->mediaVolume = 1) = 0.45 (volume is still 0.45) And call WKPageSetMediaVolume(xyz, 0.5) again then the volume gets reduced even lower. I guess this is not the intended behavior. I was trying to fix this with the below patchset.