[Webkit-unassigned] [Bug 185370] WKPageSetMediaVolume() API couldn't be used to restore the volume

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=185370

Xabier Rodríguez Calvar <calvaris at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #339715|                            |review-
              Flags|                            |

--- Comment #8 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 339715
  --> https://bugs.webkit.org/attachment.cgi?id=339715
.

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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180509/b9a1ac7d/attachment-0001.html>