Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(87)

Issue 2696893002: [Blink>Media] Add heuristic for dominant video detection for Android (Closed)

Created:
3 years, 10 months ago by Zhiqiang Zhang (Slow)
Modified:
3 years, 9 months ago
CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, Srirama
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Blink>Media] Add heuristic for dominant video detection for Android This CL adds the heuristic for dominant video detection for Android, in which we use the fullscreen state of the document and the viewport intersection to determine if a video is the dominant content in fullscreen mode (as the page may fullscreen the embedding div instead of the video element). BUG=695462 Review-Url: https://codereview.chromium.org/2696893002 Cr-Commit-Position: refs/heads/master@{#454272} Committed: https://chromium.googlesource.com/chromium/src/+/abc082487c58110e72e8d8e9ff6fa0b7abc9dd12

Patch Set 1 #

Total comments: 4

Patch Set 2 : fixed nits #

Patch Set 3 : tweaked the logic and merged tests #

Patch Set 4 : rework done #

Patch Set 5 : . #

Patch Set 6 : . #

Patch Set 7 : s/CustomControlsFullscreenDetector/MediaCustomControlsFullscreenDetector #

Total comments: 10

Patch Set 8 : addressed mlamouri's comments #

Total comments: 11

Patch Set 9 : addressed comments #

Patch Set 10 : nits #

Patch Set 11 : fixing logic in WMPI #

Total comments: 6

Patch Set 12 : addressed nits #

Total comments: 14

Patch Set 13 : addressed mlamouri@'s comments #

Patch Set 14 : fixed event listener leak #

Total comments: 3

Patch Set 15 : nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+370 lines, -4 lines) Patch
M media/blink/webmediaplayer_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M media/blink/webmediaplayer_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +5 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLVideoElement.h View 1 2 3 4 5 6 7 3 chunks +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLVideoElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +7 lines, -2 lines 0 comments Download
A third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +57 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +158 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetectorTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +126 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/WebMediaPlayer.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 78 (41 generated)
Zhiqiang Zhang (Slow)
3 years, 10 months ago (2017-02-14 20:45:51 UTC) #3
Zhiqiang Zhang (Slow)
+miu: PTAL
3 years, 10 months ago (2017-02-14 21:26:03 UTC) #5
mlamouri (slow - plz ping)
This sgtm but I would like miu@ to give some feedback. https://codereview.chromium.org/2696893002/diff/1/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): ...
3 years, 10 months ago (2017-02-15 11:08:04 UTC) #6
Zhiqiang Zhang (Slow)
https://codereview.chromium.org/2696893002/diff/1/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2696893002/diff/1/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp#newcode4149 third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4149: float yOccupationRatio = 1.0f * intersectionRect.height() / rootRect.height(); On ...
3 years, 10 months ago (2017-02-15 11:52:09 UTC) #9
Zhiqiang Zhang (Slow)
PTAL * Tweaked the logic a bit (as I missed the case where a part ...
3 years, 10 months ago (2017-02-15 17:11:42 UTC) #12
miu
+xjz
3 years, 10 months ago (2017-02-15 20:19:03 UTC) #14
miu
not lgtm On desktop, the "fills one dimension" will cause the wrong behavior for Media ...
3 years, 10 months ago (2017-02-15 20:43:56 UTC) #15
miu
I just chatted with mlamouri@ about this. It seems the main issue is that the ...
3 years, 10 months ago (2017-02-15 21:51:51 UTC) #16
xjz
On 2017/02/15 21:51:51, miu wrote: > I just chatted with mlamouri@ about this. It seems ...
3 years, 10 months ago (2017-02-15 22:27:27 UTC) #17
Zhiqiang Zhang (Slow)
I'm reworking on it after talking to mlamouri@. Will let you know when it's done.
3 years, 10 months ago (2017-02-17 16:56:29 UTC) #18
Zhiqiang Zhang (Slow)
PTAL The latest changes: * Using a helper class listening to fullscreen events and compute ...
3 years, 10 months ago (2017-02-21 14:35:40 UTC) #23
mlamouri (slow - plz ping)
Just had a quick look. Didn't check the tests and algorithm much. miu@, can you ...
3 years, 10 months ago (2017-02-21 21:58:28 UTC) #31
Zhiqiang Zhang (Slow)
miu@, xjz@: PTAL https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/120001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode47 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:47: oldDocument.removeEventListener(EventTypeNames::fullscreenchange, this, true); On 2017/02/21 at ...
3 years, 10 months ago (2017-02-22 12:29:18 UTC) #32
xjz
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode62 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:62: 1.0f * intersectionRect.width() / rootRect.width(); Can rootRect be empty? ...
3 years, 10 months ago (2017-02-22 18:48:34 UTC) #33
Zhiqiang Zhang (Slow)
PTAL Addressed xjz@'s comments. https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode62 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:62: 1.0f * intersectionRect.width() / rootRect.width(); ...
3 years, 10 months ago (2017-02-23 21:50:55 UTC) #34
xjz
lgtm % https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode62 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:62: 1.0f * intersectionRect.width() / rootRect.width(); On 2017/02/23 ...
3 years, 10 months ago (2017-02-23 22:49:46 UTC) #35
Zhiqiang Zhang (Slow)
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode62 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:62: 1.0f * intersectionRect.width() / rootRect.width(); On 2017/02/23 at 22:49:46, ...
3 years, 10 months ago (2017-02-24 12:03:40 UTC) #36
xjz
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode131 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:131: return DocumentFullscreen::fullscreenElement(videoElement().document()); On 2017/02/24 12:03:40, Zhiqiang Zhang wrote: > ...
3 years, 10 months ago (2017-02-24 18:30:10 UTC) #40
miu
lgtm % a couple small things: https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode139 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:139: Element* currentElement = ...
3 years, 10 months ago (2017-02-24 22:44:39 UTC) #41
xjz
https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/200001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode98 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:98: event->type() == EventTypeNames::loadedmetadata); Do you also want to listen ...
3 years, 10 months ago (2017-02-25 00:49:12 UTC) #42
Zhiqiang Zhang (Slow)
https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/140001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode131 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:131: return DocumentFullscreen::fullscreenElement(videoElement().document()); On 2017/02/24 at 18:30:10, xjz wrote: > ...
3 years, 10 months ago (2017-02-25 18:48:27 UTC) #45
Zhiqiang Zhang (Slow)
mlamouri: PTAL
3 years, 9 months ago (2017-02-27 11:13:42 UTC) #48
mlamouri (slow - plz ping)
lgtm with comments applied. Two follow-ups to keep in mind: - have this behind a ...
3 years, 9 months ago (2017-02-27 18:04:55 UTC) #49
Zhiqiang Zhang (Slow)
+foolip: third_party/WebKit/ +hubbe: media/ https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Source/core/html/HTMLVideoElement.cpp File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right): https://codereview.chromium.org/2696893002/diff/220001/third_party/WebKit/Source/core/html/HTMLVideoElement.cpp#newcode284 third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:284: if (m_customControlsFullscreenDetector) On 2017/02/27 at ...
3 years, 9 months ago (2017-02-28 12:16:52 UTC) #52
hubbe
media/* LGTM
3 years, 9 months ago (2017-02-28 18:23:27 UTC) #53
Zhiqiang Zhang (Slow)
+rbyers for rubber-stamping third_party/WebKit/
3 years, 9 months ago (2017-03-01 16:12:48 UTC) #55
Zhiqiang Zhang (Slow)
+haraken for rubber-stamping third_party/WebKit in APAC time
3 years, 9 months ago (2017-03-01 22:17:00 UTC) #57
haraken
Implementation-wise looks good, but do we have tests?
3 years, 9 months ago (2017-03-02 01:26:07 UTC) #58
haraken
On 2017/03/02 01:26:07, haraken wrote: > Implementation-wise looks good, but do we have tests? Sorry, ...
3 years, 9 months ago (2017-03-02 01:26:42 UTC) #59
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2696893002/260001
3 years, 9 months ago (2017-03-02 10:31:25 UTC) #62
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/321486)
3 years, 9 months ago (2017-03-02 11:37:27 UTC) #64
Zhiqiang Zhang (Slow)
mlamouri: PTAL diff #13..#14
3 years, 9 months ago (2017-03-02 14:13:29 UTC) #67
mlamouri (slow - plz ping)
lgtm with comment applied https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode102 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:102: } What about: if (event->type() ...
3 years, 9 months ago (2017-03-02 14:23:36 UTC) #70
mlamouri (slow - plz ping)
https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode102 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:102: } On 2017/03/02 at 14:23:36, mlamouri wrote: > What ...
3 years, 9 months ago (2017-03-02 14:27:19 UTC) #71
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2696893002/300001
3 years, 9 months ago (2017-03-02 14:33:08 UTC) #74
Zhiqiang Zhang (Slow)
https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp File third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp (right): https://codereview.chromium.org/2696893002/diff/280001/third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp#newcode102 third_party/WebKit/Source/core/html/MediaCustomControlsFullscreenDetector.cpp:102: } On 2017/03/02 at 14:27:18, mlamouri wrote: > On ...
3 years, 9 months ago (2017-03-02 14:33:42 UTC) #75
commit-bot: I haz the power
3 years, 9 months ago (2017-03-02 16:07:56 UTC) #78
Message was sent while issue was closed.
Committed patchset #15 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/abc082487c58110e72e8d8e9ff6f...

Powered by Google App Engine
This is Rietveld 408576698