[chromium-dev] MediaCustomControlsFullscreenDetector, timer and PiP problem

[r***@yandex-team.ru]

Hello all.

It's about this commit: https://codereview.chromium.org/2696893002.

Why use the check_viewport_intersection_timer_ timer (formerly
m_checkViewportIntersectionTimer), why not call the
Oncheckviewportintersectiontimerfried method synchronously?

Now there is such a problem with the PiP mode:
0. Android device >= 8.0, hardware navigation buttons
1. go to page, containing video and run it
2. go to full screen mode
3. immediately minimize the browser

as a result, the PIP window is not displayed. If wait a moment in #3, the
PiP window will be normally displayed.

This is because the PictureInPictureController checks
webContents.hasActiveEffectivelyFullscreenVideo() when starting PiP window;
video availability is specified by calling
VideoElement().SetIsEffectivelyFullscreen(...) in the
MediaCustomControlsFullscreenDetector::OnCheckViewportIntersectionTimerfired(...)
method. Because OnCheckViewportIntersectionTimerfired is called
asynchronously, webContents.hasActiveEffectivelyFullscreenVideo() at the
moment returns stale data.

For the experiment, I made a call to the
OnCheckViewportIntersectionTimerfired method synchronous, and PiP began to
show normally in the specified situation.

In the review, there are such comments:
xjz 2017/02/24 18:30: 10
The logic sgtm. Just ooc, if the video is the current fullscreen element,
do you
still need to start timer and check whether it is dominant? The fullscreen
might
already be a strong indicator. WDYT?
Zhiqiang Zhang (Slow) 2017/02/25 18:48: 27
That would bring in several more if-else's. I'd prefer keep the logic
simpler.

https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode131

Maybe there is some right way to get rid of the timer and delay?

['Mounir Lamouri' via Chromium-dev]

If I understand correctly, what you are suggesting is to run the checks
immediately when the video is fullscreen compared to when the video is
inside a fullscreen element?

Post by r***@yandex-team.ru
Hello all.
It's about this commit: https://codereview.chromium.org/2696893002.
Why use the check_viewport_intersection_timer_ timer (formerly
m_checkViewportIntersectionTimer), why not call the
Oncheckviewportintersectiontimerfried method synchronously?
0. Android device >= 8.0, hardware navigation buttons
1. go to page, containing video and run it
2. go to full screen mode
3. immediately minimize the browser
as a result, the PIP window is not displayed. If wait a moment in #3, the
PiP window will be normally displayed.
This is because the PictureInPictureController checks
webContents.hasActiveEffectivelyFullscreenVideo() when starting PiP window;
video availability is specified by calling
VideoElement().SetIsEffectivelyFullscreen(...) in the
MediaCustomControlsFullscreenDetector::OnCheckViewportIntersectionTimerfired(...)
method. Because OnCheckViewportIntersectionTimerfired is called
asynchronously, webContents.hasActiveEffectivelyFullscreenVideo() at the
moment returns stale data.
For the experiment, I made a call to the
OnCheckViewportIntersectionTimerfired method synchronous, and PiP began to
show normally in the specified situation.
xjz 2017/02/24 18:30: 10
The logic sgtm. Just ooc, if the video is the current fullscreen element,
do you
still need to start timer and check whether it is dominant? The fullscreen
might
already be a strong indicator. WDYT?
Zhiqiang Zhang (Slow) 2017/02/25 18:48: 27
That would bring in several more if-else's. I'd prefer keep the logic
simpler.
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode131
Maybe there is some right way to get rid of the timer and delay?
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/667bae95-0193-4628-9d8a-dc376577df90%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/667bae95-0193-4628-9d8a-dc376577df90%40chromium.org?utm_medium=email&utm_source=footer>
.

[r***@yandex-team.ru]

I don't have any suggestion yet.

In fact, my experiment with synchronous call to
OnCheckViewportIntersectionTimerFired does not work 100% of the cases.
I also tried to add EventTypeNames::resize event handling but with no luck.

In my tests I receive events in such order:

EventTypeNames::loadedmetadata

EventTypeNames::resize

EventTypeNames::webkitfullscreenchange


VideoElement may be dominant (as shown by the call
ComputeIsDominantVideoForTests) at the time of receiving event
EventTypeNames::webkitfullscreenchange, but may be later, and the latter
case is covered only by timer.

So instead of suggestions I have a question - is there any way to get rid
of the timer? Is it possible to catch the transition to full screen mode
without delay?

втПрМОк, 27 МПября 2018 г., 2:53:17 UTC+3 пПльзПватель Mounir Lamouri

Post by 'Mounir Lamouri' via Chromium-dev
If I understand correctly, what you are suggesting is to run the checks
immediately when the video is fullscreen compared to when the video is
inside a fullscreen element?

Post by r***@yandex-team.ru
Hello all.
It's about this commit: https://codereview.chromium.org/2696893002.
Why use the check_viewport_intersection_timer_ timer (formerly
m_checkViewportIntersectionTimer), why not call the
Oncheckviewportintersectiontimerfried method synchronously?
0. Android device >= 8.0, hardware navigation buttons
1. go to page, containing video and run it
2. go to full screen mode
3. immediately minimize the browser
as a result, the PIP window is not displayed. If wait a moment in #3, the
PiP window will be normally displayed.
This is because the PictureInPictureController checks
webContents.hasActiveEffectivelyFullscreenVideo() when starting PiP window;
video availability is specified by calling
VideoElement().SetIsEffectivelyFullscreen(...) in the
MediaCustomControlsFullscreenDetector::OnCheckViewportIntersectionTimerfired(...)
method. Because OnCheckViewportIntersectionTimerfired is called
asynchronously, webContents.hasActiveEffectivelyFullscreenVideo() at the
moment returns stale data.
For the experiment, I made a call to the
OnCheckViewportIntersectionTimerfired method synchronous, and PiP began to
show normally in the specified situation.
xjz 2017/02/24 18:30: 10
The logic sgtm. Just ooc, if the video is the current fullscreen element,
do you
still need to start timer and check whether it is dominant? The
fullscreen might
already be a strong indicator. WDYT?
Zhiqiang Zhang (Slow) 2017/02/25 18:48: 27
That would bring in several more if-else's. I'd prefer keep the logic
simpler.
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode131
Maybe there is some right way to get rid of the timer and delay?
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/667bae95-0193-4628-9d8a-dc376577df90%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/667bae95-0193-4628-9d8a-dc376577df90%40chromium.org?utm_medium=email&utm_source=footer>
.

['Mounir Lamouri' via Chromium-dev]

Post by r***@yandex-team.ru
I don't have any suggestion yet.
In fact, my experiment with synchronous call to
OnCheckViewportIntersectionTimerFired does not work 100% of the cases.
I also tried to add EventTypeNames::resize event handling but with no luck.
EventTypeNames::loadedmetadata
EventTypeNames::resize
EventTypeNames::webkitfullscreenchange
VideoElement may be dominant (as shown by the call
ComputeIsDominantVideoForTests) at the time of receiving event
EventTypeNames::webkitfullscreenchange, but may be later, and the latter
case is covered only by timer.
So instead of suggestions I have a question - is there any way to get rid
of the timer? Is it possible to catch the transition to full screen mode
without delay?

If I remember correctly, the timer was added to make sure that when we do
the checks all the layout changes are in place and we don't end up with
some race conditions that would make the feature never work on the page. We
never tried to tweak the timer as no one ever complained before so maybe we
could reduce it or thing about other solutions.

Though, it might be a good idea to file a bug (crbug.com/new) and continue
the discussion there. Feel free to CC me.

Post by r***@yandex-team.ru
втПрМОк, 27 МПября 2018 г., 2:53:17 UTC+3 пПльзПватель Mounir Lamouri

Post by 'Mounir Lamouri' via Chromium-dev
If I understand correctly, what you are suggesting is to run the checks
immediately when the video is fullscreen compared to when the video is
inside a fullscreen element?

Post by r***@yandex-team.ru
Hello all.
It's about this commit: https://codereview.chromium.org/2696893002.
Why use the check_viewport_intersection_timer_ timer (formerly
m_checkViewportIntersectionTimer), why not call the
Oncheckviewportintersectiontimerfried method synchronously?
0. Android device >= 8.0, hardware navigation buttons
1. go to page, containing video and run it
2. go to full screen mode
3. immediately minimize the browser
as a result, the PIP window is not displayed. If wait a moment in #3,
the PiP window will be normally displayed.
This is because the PictureInPictureController checks
webContents.hasActiveEffectivelyFullscreenVideo() when starting PiP window;
video availability is specified by calling
VideoElement().SetIsEffectivelyFullscreen(...) in the
MediaCustomControlsFullscreenDetector::OnCheckViewportIntersectionTimerfired(...)
method. Because OnCheckViewportIntersectionTimerfired is called
asynchronously, webContents.hasActiveEffectivelyFullscreenVideo() at the
moment returns stale data.
For the experiment, I made a call to the
OnCheckViewportIntersectionTimerfired method synchronous, and PiP began to
show normally in the specified situation.
xjz 2017/02/24 18:30: 10
The logic sgtm. Just ooc, if the video is the current fullscreen
element, do you
still need to start timer and check whether it is dominant? The
fullscreen might
already be a strong indicator. WDYT?
Zhiqiang Zhang (Slow) 2017/02/25 18:48: 27
That would bring in several more if-else's. I'd prefer keep the logic
simpler.
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode131
Maybe there is some right way to get rid of the timer and delay?
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/667bae95-0193-4628-9d8a-dc376577df90%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/667bae95-0193-4628-9d8a-dc376577df90%40chromium.org?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/7f5ee1ad-ddcd-48c8-a5d2-110891d47698%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/7f5ee1ad-ddcd-48c8-a5d2-110891d47698%40chromium.org?utm_medium=email&utm_source=footer>
.

[Yuri Wiitala]

BTW--My recent code change (still need to redo it, according to some Blink
owners) is very related to all this:
https://chromium-review.googlesource.com/c/chromium/src/+/1341086 My change
would affect PiP as well, since some logic is shared with the Media
Remoting UI. I probably won't be able to get back to this for about another
week. I'd be grateful if one of you could take-over and finish it for me,
as I'm very oversubscribed at the moment.

Post by 'Mounir Lamouri' via Chromium-dev

Post by r***@yandex-team.ru
I don't have any suggestion yet.
In fact, my experiment with synchronous call to
OnCheckViewportIntersectionTimerFired does not work 100% of the cases.
I also tried to add EventTypeNames::resize event handling but with no luck.
EventTypeNames::loadedmetadata
EventTypeNames::resize
EventTypeNames::webkitfullscreenchange
VideoElement may be dominant (as shown by the call
ComputeIsDominantVideoForTests) at the time of receiving event
EventTypeNames::webkitfullscreenchange, but may be later, and the latter
case is covered only by timer.
So instead of suggestions I have a question - is there any way to get rid
of the timer? Is it possible to catch the transition to full screen mode
without delay?

If I remember correctly, the timer was added to make sure that when we do
the checks all the layout changes are in place and we don't end up with
some race conditions that would make the feature never work on the page. We
never tried to tweak the timer as no one ever complained before so maybe we
could reduce it or thing about other solutions.
Though, it might be a good idea to file a bug (crbug.com/new) and
continue the discussion there. Feel free to CC me.

Post by r***@yandex-team.ru
втПрМОк, 27 МПября 2018 г., 2:53:17 UTC+3 пПльзПватель Mounir Lamouri

Post by 'Mounir Lamouri' via Chromium-dev
If I understand correctly, what you are suggesting is to run the checks
immediately when the video is fullscreen compared to when the video is
inside a fullscreen element?

Post by r***@yandex-team.ru
Hello all.
It's about this commit: https://codereview.chromium.org/2696893002.
Why use the check_viewport_intersection_timer_ timer (formerly
m_checkViewportIntersectionTimer), why not call the
Oncheckviewportintersectiontimerfried method synchronously?
0. Android device >= 8.0, hardware navigation buttons
1. go to page, containing video and run it
2. go to full screen mode
3. immediately minimize the browser
as a result, the PIP window is not displayed. If wait a moment in #3,
the PiP window will be normally displayed.
This is because the PictureInPictureController checks
webContents.hasActiveEffectivelyFullscreenVideo() when starting PiP window;
video availability is specified by calling
VideoElement().SetIsEffectivelyFullscreen(...) in the
MediaCustomControlsFullscreenDetector::OnCheckViewportIntersectionTimerfired(...)
method. Because OnCheckViewportIntersectionTimerfired is called
asynchronously, webContents.hasActiveEffectivelyFullscreenVideo() at the
moment returns stale data.
For the experiment, I made a call to the
OnCheckViewportIntersectionTimerfired method synchronous, and PiP began to
show normally in the specified situation.
xjz 2017/02/24 18:30: 10
The logic sgtm. Just ooc, if the video is the current fullscreen
element, do you
still need to start timer and check whether it is dominant? The
fullscreen might
already be a strong indicator. WDYT?
Zhiqiang Zhang (Slow) 2017/02/25 18:48: 27
That would bring in several more if-else's. I'd prefer keep the logic
simpler.
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode131
Maybe there is some right way to get rid of the timer and delay?
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/667bae95-0193-4628-9d8a-dc376577df90%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/667bae95-0193-4628-9d8a-dc376577df90%40chromium.org?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups
"Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/7f5ee1ad-ddcd-48c8-a5d2-110891d47698%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/7f5ee1ad-ddcd-48c8-a5d2-110891d47698%40chromium.org?utm_medium=email&utm_source=footer>
.

[r***@yandex-team.ru]

Thank you, colleagues. Let's continue the discussion in the bug
https://bugs.chromium.org/p/chromium/issues/detail?id=911023

Something went wrong and the bug was created with OS=Linux. Unfortunately I
don't see how I could edit the description, or add a CC. I will be grateful
if someone will correct it, or tell me how I can do it.

Post by Yuri Wiitala
BTW--My recent code change (still need to redo it, according to some Blink
https://chromium-review.googlesource.com/c/chromium/src/+/1341086 My
change would affect PiP as well, since some logic is shared with the Media
Remoting UI. I probably won't be able to get back to this for about another
week. I'd be grateful if one of you could take-over and finish it for me,
as I'm very oversubscribed at the moment.

Post by 'Mounir Lamouri' via Chromium-dev

Post by r***@yandex-team.ru
I don't have any suggestion yet.
In fact, my experiment with synchronous call to
OnCheckViewportIntersectionTimerFired does not work 100% of the cases.
I also tried to add EventTypeNames::resize event handling but with no luck.
EventTypeNames::loadedmetadata
EventTypeNames::resize
EventTypeNames::webkitfullscreenchange
VideoElement may be dominant (as shown by the call
ComputeIsDominantVideoForTests) at the time of receiving event
EventTypeNames::webkitfullscreenchange, but may be later, and the latter
case is covered only by timer.
So instead of suggestions I have a question - is there any way to get
rid of the timer? Is it possible to catch the transition to full screen
mode without delay?

If I remember correctly, the timer was added to make sure that when we do
the checks all the layout changes are in place and we don't end up with
some race conditions that would make the feature never work on the page. We
never tried to tweak the timer as no one ever complained before so maybe we
could reduce it or thing about other solutions.
Though, it might be a good idea to file a bug (crbug.com/new) and
continue the discussion there. Feel free to CC me.

Post by r***@yandex-team.ru
втПрМОк, 27 МПября 2018 г., 2:53:17 UTC+3 пПльзПватель Mounir Lamouri

Post by 'Mounir Lamouri' via Chromium-dev
If I understand correctly, what you are suggesting is to run the checks
immediately when the video is fullscreen compared to when the video is
inside a fullscreen element?

Post by r***@yandex-team.ru
Hello all.
It's about this commit: https://codereview.chromium.org/2696893002.
Why use the check_viewport_intersection_timer_ timer (formerly
m_checkViewportIntersectionTimer), why not call the
Oncheckviewportintersectiontimerfried method synchronously?
0. Android device >= 8.0, hardware navigation buttons
1. go to page, containing video and run it
2. go to full screen mode
3. immediately minimize the browser
as a result, the PIP window is not displayed. If wait a moment in #3,
the PiP window will be normally displayed.
This is because the PictureInPictureController checks
webContents.hasActiveEffectivelyFullscreenVideo() when starting PiP window;
video availability is specified by calling
VideoElement().SetIsEffectivelyFullscreen(...) in the
MediaCustomControlsFullscreenDetector::OnCheckViewportIntersectionTimerfired(...)
method. Because OnCheckViewportIntersectionTimerfired is called
asynchronously, webContents.hasActiveEffectivelyFullscreenVideo() at the
moment returns stale data.
For the experiment, I made a call to the
OnCheckViewportIntersectionTimerfired method synchronous, and PiP began to
show normally in the specified situation.
xjz 2017/02/24 18:30: 10
The logic sgtm. Just ooc, if the video is the current fullscreen
element, do you
still need to start timer and check whether it is dominant? The
fullscreen might
already be a strong indicator. WDYT?
Zhiqiang Zhang (Slow) 2017/02/25 18:48: 27
That would bring in several more if-else's. I'd prefer keep the logic
simpler.
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode131
Maybe there is some right way to get rid of the timer and delay?
--
--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/667bae95-0193-4628-9d8a-dc376577df90%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/667bae95-0193-4628-9d8a-dc376577df90%40chromium.org?utm_medium=email&utm_source=footer>
.

--

--
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google
Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/7f5ee1ad-ddcd-48c8-a5d2-110891d47698%40chromium.org
<https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/7f5ee1ad-ddcd-48c8-a5d2-110891d47698%40chromium.org?utm_medium=email&utm_source=footer>
.