New issue
Advanced search Search tips

Issue 871417 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 21
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-08-20
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 876466



Sign in to add a comment

Optimize WebGL video upload sync tokens

Project Member Reported by kainino@chromium.org, Aug 6

Issue description

It seems like verified sync tokens are being used in WebGL video upload (see paint_canvas_video_renderer.cc) when we could use unverified sync tokens. We should look into whether we can change this - it would provide a huge speedup to some video upload paths.
 
Blockedon: 819914
Components: Internals>GPU>Video
Labels: -Pri-3 Pri-2
Status: Assigned (was: Started)
Blocked on the fixes in  issue 819914 ; I'll get back to this when they're done.

The WIP change is here:
https://chromium-review.googlesource.com/c/chromium/src/+/1164163
NextAction: 2018-08-20
The NextAction date has arrived: 2018-08-20
Status: Fixed (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/228446ba19c6a6f6cd7b61402002b882523cfe07

commit 228446ba19c6a6f6cd7b61402002b882523cfe07
Author: Kai Ninomiya <kainino@chromium.org>
Date: Tue Aug 21 18:23:01 2018

Use unverified sync tokens in PaintCanvasVideoRenderer

PaintCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture
unnecessarily used verified sync tokens to pass textures between
Skia's context and WebGL's context. Verification isn't needed
because these two clients are being used synchronously from the
same process and thread.

Changing these sync tokens to be unverified gives us a nice
speedup - on a simple mp4-to-WebGL case on Linux, the texImage2D
blocking time drops from ~1.25 ms to ~0.70ms (~80% speedup).

Bug:  871417 
Change-Id: I38c246fdb49588eccc87211fa06a2abf00e55c5c
Reviewed-on: https://chromium-review.googlesource.com/1164163
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584840}
[modify] https://crrev.com/228446ba19c6a6f6cd7b61402002b882523cfe07/media/renderers/paint_canvas_video_renderer.cc

Great work Kai!

Blockedon: -867368 -819914
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de9497de9793e055da360fcadcf5e26da9f3262b

commit de9497de9793e055da360fcadcf5e26da9f3262b
Author: Kai Ninomiya <kainino@chromium.org>
Date: Mon Aug 27 23:49:04 2018

Revert "Use unverified sync tokens in PaintCanvasVideoRenderer"

This reverts commit 228446ba19c6a6f6cd7b61402002b882523cfe07.

Reason for revert: Potentially causing flakes in virtual/android/fullscreen/video-fixed-background.html

Bug: 876466

Original change's description:
> Use unverified sync tokens in PaintCanvasVideoRenderer
> 
> PaintCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture
> unnecessarily used verified sync tokens to pass textures between
> Skia's context and WebGL's context. Verification isn't needed
> because these two clients are being used synchronously from the
> same process and thread.
> 
> Changing these sync tokens to be unverified gives us a nice
> speedup - on a simple mp4-to-WebGL case on Linux, the texImage2D
> blocking time drops from ~1.25 ms to ~0.70ms (~80% speedup).
> 
> Bug:  871417 
> Change-Id: I38c246fdb49588eccc87211fa06a2abf00e55c5c
> Reviewed-on: https://chromium-review.googlesource.com/1164163
> Reviewed-by: Dan Sanders <sandersd@chromium.org>
> Commit-Queue: Kai Ninomiya <kainino@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#584840}

TBR=sandersd@chromium.org,kainino@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  871417 
Change-Id: I51d3e5f9308eadf08babec4e1c99b1d51c4a3b90
Reviewed-on: https://chromium-review.googlesource.com/1192129
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586500}
[modify] https://crrev.com/de9497de9793e055da360fcadcf5e26da9f3262b/media/renderers/paint_canvas_video_renderer.cc

Blocking: 876466
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c9fce27cac9d96085bd8bd29e345dc2c6b378a25

commit c9fce27cac9d96085bd8bd29e345dc2c6b378a25
Author: Kai Ninomiya <kainino@chromium.org>
Date: Mon Sep 03 01:04:41 2018

Reland "Use unverified sync tokens in PaintCanvasVideoRenderer"

This reverts commit de9497de9793e055da360fcadcf5e26da9f3262b.

Reason for revert: flake continued after revert,
so this change wasn't responsible.

Original change's description:
> Revert "Use unverified sync tokens in PaintCanvasVideoRenderer"
> 
> This reverts commit 228446ba19c6a6f6cd7b61402002b882523cfe07.
> 
> Reason for revert: Potentially causing flakes in virtual/android/fullscreen/video-fixed-background.html
> 
> Bug: 876466
> 
> Original change's description:
> > Use unverified sync tokens in PaintCanvasVideoRenderer
> > 
> > PaintCanvasVideoRenderer::CopyVideoFrameTexturesToGLTexture
> > unnecessarily used verified sync tokens to pass textures between
> > Skia's context and WebGL's context. Verification isn't needed
> > because these two clients are being used synchronously from the
> > same process and thread.
> > 
> > Changing these sync tokens to be unverified gives us a nice
> > speedup - on a simple mp4-to-WebGL case on Linux, the texImage2D
> > blocking time drops from ~1.25 ms to ~0.70ms (~80% speedup).
> > 
> > Bug:  871417 
> > Change-Id: I38c246fdb49588eccc87211fa06a2abf00e55c5c
> > Reviewed-on: https://chromium-review.googlesource.com/1164163
> > Reviewed-by: Dan Sanders <sandersd@chromium.org>
> > Commit-Queue: Kai Ninomiya <kainino@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#584840}
> 
> TBR=sandersd@chromium.org,kainino@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug:  871417 
> Change-Id: I51d3e5f9308eadf08babec4e1c99b1d51c4a3b90
> Reviewed-on: https://chromium-review.googlesource.com/1192129
> Reviewed-by: Kai Ninomiya <kainino@chromium.org>
> Commit-Queue: Kai Ninomiya <kainino@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#586500}

TBR=sandersd@chromium.org,kainino@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 876466,  871417 
Change-Id: Ic5c2206901239695b8289f0fa3e63f925094444d
Reviewed-on: https://chromium-review.googlesource.com/1200487
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588308}
[modify] https://crrev.com/c9fce27cac9d96085bd8bd29e345dc2c6b378a25/media/renderers/paint_canvas_video_renderer.cc

Sign in to add a comment