Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1236313002:
Use skia to do accelerated YUV conversion for rec709 'HD' color space in (Closed)
|
Created:
5 years, 5 months ago by rileya (GONE FROM CHROMIUM) Modified:
5 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse skia to do accelerated YUV conversion for rec709 'HD' color space in
the video -> accelerated canvas copy path.
Motivated by https://codereview.chromium.org/1230593005, which makes 709
the default for MSE. Without the hardware-accelerated YUV convert path,
video -> WebGL copies are pretty slow (in particular this degrades
performance for playing 360 videos in YouTube).
Must land after https://codereview.chromium.org/1241723005 lands and
rolls.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel
BUG=333619
Committed: https://crrev.com/e5045e2a19a96c6f1a37e638ae5d4228b90757be
Cr-Commit-Position: refs/heads/master@{#340905}
Patch Set 1 #
Messages
Total messages: 23 (5 generated)
rileya@chromium.org changed reviewers: + dalecurtis@chromium.org, dcastagna@chromium.org, watk@chromium.org
A change that I forgot to do way back when...
On 2015/07/15 at 01:37:45, rileya wrote: > A change that I forgot to do way back when... Cool. How did you test this?
lgtm
lgtm. Thanks!
On 2015/07/15 21:00:13, watk wrote: > lgtm. Thanks! @watk, this is vestigial to the issue 333619 work, right?
noel@chromium.org changed reviewers: + noel@chromium.org
On 2015/07/23 06:10:55, noel gordon wrote: > On 2015/07/15 21:00:13, watk wrote: > > lgtm. Thanks! > > @watk, this is vestigial to the issue 333619 work, right? It's important from a performance perspective. i.e., this shouldn't make any difference visually, but it makes copying from bt709 frames to webgl textures efficient.
> How did you test this? I ran the blackwhite.html media_browsertests, and also manually checked the image diffs to ensure it didn't introduce any more error than any other color space showed with the GPU path. And I just double-checked this again now that the skia change has rolled.
#8 Nod, the canvas.getContext('3d').drawImage(<video>) case. The canvas 2d case
looks like it might benefit as well.
#9 The Skia GM also has a test (in https://codereview.chromium.org/1241723005)
Are there any blink layout tests?
Also, add a BUG= line and the following line to your change description:
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel
On 2015/07/28 04:54:17, noel gordon wrote: > Are there any blink layout tests? Not that I'm aware of, I'm not sure what sort of coverage the video->canvas path has there. > Also, add a BUG= line and the following line to your change description: Done.
On 2015/07/28 23:49:27, rileya wrote: > On 2015/07/28 04:54:17, noel gordon wrote: > > Are there any blink layout tests? > > Not that I'm aware of, I'm not sure what sort of coverage the video->canvas path > has there. Chromium code search is amazing :) https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Seems to be a bunch of LayoutTests/fast/canvas/webgl/tex-image-and-sub-image-2d-with-video* tests. > > Also, add a BUG= line and the following line to your change description: > > Done. OK thanks. LGTM.
On 2015/07/29 at 00:29:02, noel wrote: > On 2015/07/28 23:49:27, rileya wrote: > > On 2015/07/28 04:54:17, noel gordon wrote: > > > Are there any blink layout tests? > > > > Not that I'm aware of, I'm not sure what sort of coverage the video->canvas path > > has there. > > Chromium code search is amazing :) > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Seems to be a bunch of LayoutTests/fast/canvas/webgl/tex-image-and-sub-image-2d-with-video* tests. > There is also canvas/yuv-video-on-accelerated-canvas.html I added the trybot linux_blink_rel to see if it passes since it's giving me problems on crrev.com/1144323003 > > > Also, add a BUG= line and the following line to your change description: > > > > Done. > > OK thanks. LGTM.
On 2015/07/29 04:25:56, Daniele Castagna wrote: > There is also canvas/yuv-video-on-accelerated-canvas.html > I added the trybot linux_blink_rel to see if it passes since it's giving me > problems on crrev.com/1144323003 Thank you. And I expect there are more. Generally, we can dry-run the patch to sniff out any problems on the Blink side.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236313002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1236313002/1
On 2015/07/29 04:25:56, Daniele Castagna wrote: > There is also canvas/yuv-video-on-accelerated-canvas.html > I added the trybot linux_blink_rel to see if it passes since it's giving me > problems on crrev.com/1144323003 @Daniele Passed for this CL. Perhaps your change has some other problem (I'm not sure what it is, but it look like a blink image failure on a yuv gpu video test).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rileya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236313002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1236313002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/e5045e2a19a96c6f1a37e638ae5d4228b90757be Cr-Commit-Position: refs/heads/master@{#340905} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
