Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
595553002:
Use the skmultipicture to perform GPU rasterization (Closed)
Descriptioncc: Use SkMultiPicture for GPU rasterization
GpuRasterWorkerPool holds an SkMultiPicture, the RasterBufferImpl is
created with a pointer to SkMultiPicture, and returns the
SkPictureRecorder's canvas when AquireSkCanvas is called. When the
canvas is released, we get the surface canvas and pass it to the
multipicture along with the recorded picture.
When we're finished with all of the tiles, we draw the multipicture.
Updated Picture::Raster() to call canvas->drawPicture(), instead of
picture->draw(). This is required to prevent parsing the skia ops
twice when using the multipicturedraw, since picture->draw() always
parses the ops, whereas canvas->drawPicture() might parse the ops,
or it might simply place the whole picture on the canvas (as it will
for the recorder's canvas)
BUG=skia:2315
Committed: https://crrev.com/04cea97063df4790c91607bce25000c07119028e
Cr-Commit-Position: refs/heads/master@{#296227}
Patch Set 1 #
Total comments: 12
Patch Set 2 : If we don't have a surface, return a nullcanvas #Patch Set 3 : Fixed comment and bad if statement #
Messages
Total messages: 21 (3 generated)
hendrikw@chromium.org changed reviewers: + bsalomon@chromium.org, ernstm@chromium.org, reveman@chromium.org, robertphillips@chromium.org
PTAL
robertphillips@google.com changed reviewers: + robertphillips@google.com
The Skia calls look good but I defer to others for the embedding in gpu_raster_worker_pool.
https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.cc:45: canvas->save(); do we still need this save/restore pair? https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.cc:55: : skia::AdoptRef(SkCreateNullCanvas()); it seems a bit silly to add a null canvas. would it make more sense to not add the picture when surface_ is NULL? https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.cc:239: multi_picture_draw_.draw(); Does calling draw() also clear the SkMultiPictureDraw? Maybe do that explicitly instead or add a comment.. https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc#newc... cc/resources/picture.cc:357: } I assume this is the fix for picture ops being being parsed twice as mentioned in the description. Can you move this to a separate patch and explain a bit more why the old code is wrong? I'm not sure I understand exactly why this is needed..
https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.cc:45: canvas->save(); On 2014/09/22 21:29:05, reveman wrote: > do we still need this save/restore pair? It is probably still required, but should be moved to wrap around the canvas created below, since that's the canvas we need to keep in a good state. I'll make the change. https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.cc:55: : skia::AdoptRef(SkCreateNullCanvas()); On 2014/09/22 21:29:05, reveman wrote: > it seems a bit silly to add a null canvas. would it make more sense to not add > the picture when surface_ is NULL? If we want status quo, I should probably allocate the surface canvas in the AquireSkCanvas, and if the surface is missing, do not bother creating the recorder canvas, and return the null canvas there. That means I would need to keep a pointer to both canvases, which isn't a big deal. https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.cc:239: multi_picture_draw_.draw(); On 2014/09/22 21:29:05, reveman wrote: > Does calling draw() also clear the SkMultiPictureDraw? Maybe do that explicitly > instead or add a comment.. Yes, it clears. I'll add a comment. https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc#newc... cc/resources/picture.cc:357: } On 2014/09/22 21:29:05, reveman wrote: > I assume this is the fix for picture ops being being parsed twice as mentioned > in the description. Can you move this to a separate patch and explain a bit more > why the old code is wrong? I'm not sure I understand exactly why this is > needed.. I'd rather not separate these. I can put more details in the comments if you like. Essentially, picture->draw will parse the ops --and-- canvas->drawPicture could parse the ops, or it could put the whole picture in the canvas as one operation, depending on the canvas implementation. We need this for the SkMultiPicture because we'll end up parsing the ops one extra time, since we parse the ops when we call picture->draw(), then again when we SkMultiPicture->draw()
On 2014/09/22 21:48:39, Hendrik wrote: > https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... > File cc/resources/gpu_raster_worker_pool.cc (right): > > https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... > cc/resources/gpu_raster_worker_pool.cc:45: canvas->save(); > On 2014/09/22 21:29:05, reveman wrote: > > do we still need this save/restore pair? > > It is probably still required, but should be moved to wrap around the canvas > created below, since that's the canvas we need to keep in a good state. > > I'll make the change. > > https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... > cc/resources/gpu_raster_worker_pool.cc:55: : > skia::AdoptRef(SkCreateNullCanvas()); > On 2014/09/22 21:29:05, reveman wrote: > > it seems a bit silly to add a null canvas. would it make more sense to not add > > the picture when surface_ is NULL? > > If we want status quo, I should probably allocate the surface canvas in the > AquireSkCanvas, and if the surface is missing, do not bother creating the > recorder canvas, and return the null canvas there. > > That means I would need to keep a pointer to both canvases, which isn't a big > deal. > > https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... > cc/resources/gpu_raster_worker_pool.cc:239: multi_picture_draw_.draw(); > On 2014/09/22 21:29:05, reveman wrote: > > Does calling draw() also clear the SkMultiPictureDraw? Maybe do that > explicitly > > instead or add a comment.. > > Yes, it clears. I'll add a comment. > > https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc > File cc/resources/picture.cc (right): > > https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc#newc... > cc/resources/picture.cc:357: } > On 2014/09/22 21:29:05, reveman wrote: > > I assume this is the fix for picture ops being being parsed twice as mentioned > > in the description. Can you move this to a separate patch and explain a bit > more > > why the old code is wrong? I'm not sure I understand exactly why this is > > needed.. > > I'd rather not separate these. I can put more details in the comments if you > like. > > Essentially, picture->draw will parse the ops --and-- canvas->drawPicture could > parse the ops, or it could put the whole picture in the canvas as one operation, > depending on the canvas implementation. > > We need this for the SkMultiPicture because we'll end up parsing the ops one > extra time, since we parse the ops when we call picture->draw(), then again when > we SkMultiPicture->draw() Hmm, after looking a bit closer, we probably do want to keep the save and restore where they are, so that when we call multipicture->draw(), it calls save/restore and keeps the state of the passed in canvas intact. I probably don't need the extra canvas pointer either.
Updated to return the null canvas immediately if we're missing the surface, added a comment, and updated the description to explain why we need to call canvas->drawPicture when we start using SkMultiPictureDraw
lgtm after adding a comment to picture.cc https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/595553002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.cc:45: canvas->save(); On 2014/09/22 21:48:39, Hendrik wrote: > On 2014/09/22 21:29:05, reveman wrote: > > do we still need this save/restore pair? > > It is probably still required, but should be moved to wrap around the canvas > created below, since that's the canvas we need to keep in a good state. > > I'll make the change. Acknowledged. https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc#newc... cc/resources/picture.cc:357: } On 2014/09/22 21:48:39, Hendrik wrote: > On 2014/09/22 21:29:05, reveman wrote: > > I assume this is the fix for picture ops being being parsed twice as mentioned > > in the description. Can you move this to a separate patch and explain a bit > more > > why the old code is wrong? I'm not sure I understand exactly why this is > > needed.. > > I'd rather not separate these. I can put more details in the comments if you > like. > > Essentially, picture->draw will parse the ops --and-- canvas->drawPicture could > parse the ops, or it could put the whole picture in the canvas as one operation, > depending on the canvas implementation. > > We need this for the SkMultiPicture because we'll end up parsing the ops one > extra time, since we parse the ops when we call picture->draw(), then again when > we SkMultiPicture->draw() Ok, a comment here would be useful as it's not obvious what the differences between these calls are. Maybe just make it clear that drawPicture is preferred when possible as it's in some cases more efficient. Btw, why doesn't drawPicture take a callback?
https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc#newc... cc/resources/picture.cc:357: } We have renamed SkPicture::draw to be SkPicture::playback in Skia (and will be pushing that change out to Chrome soon). So ... SkPicture::playback replays the picture into another canvas and is intended for clients that want to see all the operations replayed (e.g., AnalysisCanvas). Interruption of playback is supported for exactly the AnalysisCanvas' early out behavior. SkPicture::playback will always unfurl the picture! SkCanvas::drawPicture is intended for when the client wants to draw the picture. In this case it is assumed the client wants to draw the entire picture and not interrupt it at some arbitrary point (and leave the receiving canvas in some odd state). Since the SkPicture is passed in as an object the target canvases can ref it if needed (e.g., the recording canvas from SkPictureRecorder::beginRecording) rather then unfurling it.
https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc#newc... cc/resources/picture.cc:357: } On 2014/09/23 12:54:55, robertphillips wrote: > We have renamed SkPicture::draw to be SkPicture::playback in Skia (and will be > pushing that change out to Chrome soon). So ... > > SkPicture::playback replays the picture into another canvas and is intended for > clients that want to see all the operations replayed (e.g., AnalysisCanvas). > Interruption of playback is supported for exactly the AnalysisCanvas' early out > behavior. SkPicture::playback will always unfurl the picture! > > SkCanvas::drawPicture is intended for when the client wants to draw the picture. > In this case it is assumed the client wants to draw the entire picture and not > interrupt it at some arbitrary point (and leave the receiving canvas in some odd > state). Since the SkPicture is passed in as an object the target canvases can > ref it if needed (e.g., the recording canvas from > SkPictureRecorder::beginRecording) rather then unfurling it. Ok, that makes sense. We should probably change the cc::Picture API to better match skia instead of trying to do everything in Picture::Raster(). Maybe split it into Picture::Draw and Picture::Playback.
On 2014/09/23 14:59:46, reveman wrote: > https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc > File cc/resources/picture.cc (right): > > https://codereview.chromium.org/595553002/diff/1/cc/resources/picture.cc#newc... > cc/resources/picture.cc:357: } > On 2014/09/23 12:54:55, robertphillips wrote: > > We have renamed SkPicture::draw to be SkPicture::playback in Skia (and will be > > pushing that change out to Chrome soon). So ... > > > > SkPicture::playback replays the picture into another canvas and is intended > for > > clients that want to see all the operations replayed (e.g., AnalysisCanvas). > > Interruption of playback is supported for exactly the AnalysisCanvas' early > out > > behavior. SkPicture::playback will always unfurl the picture! > > > > SkCanvas::drawPicture is intended for when the client wants to draw the > picture. > > In this case it is assumed the client wants to draw the entire picture and not > > interrupt it at some arbitrary point (and leave the receiving canvas in some > odd > > state). Since the SkPicture is passed in as an object the target canvases can > > ref it if needed (e.g., the recording canvas from > > SkPictureRecorder::beginRecording) rather then unfurling it. > > Ok, that makes sense. We should probably change the cc::Picture API to better > match skia instead of trying to do everything in Picture::Raster(). Maybe split > it into Picture::Draw and Picture::Playback. That's probably a good idea. Robert, should I wait for the skia roll before committing this? There will be conflicts. How soon do you want this in? I also noticed that Picture::Replay calls picture_->draw(), currently only used by benchmarking using SkDebugCanvas, I wonder if I should also change this to use drawPicture?
Fixed comments and wrong early return
lgtm but please adjust the description before landing. it should preferably be in this format: component: One-line description of change. More detailed description that wraps at 76 columns or less. BUG=
> Robert, should I wait for the skia roll before committing this? There will be > conflicts. How soon do you want this in? I will make the change after you land so don't wait on a Skia roll. > I also noticed that Picture::Replay calls picture_->draw(), currently only used > by benchmarking using SkDebugCanvas, I wonder if I should also change this to > use drawPicture? Hmmm. The SkDebugCanvas probably shouldn't be used for benchmarking. Most likely the intent is to actually unfurl the sub-picture into the SkDebugCanvas so that all the commands can be converted to a string. Moving forward (in a paint slimming world) this will cause problems since it will flatten the picture hierarchy.
The CQ bit was checked by hendrikw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/595553002/40001
On 2014/09/23 16:43:41, robertphillips wrote: > > Robert, should I wait for the skia roll before committing this? There will be > > conflicts. How soon do you want this in? > > I will make the change after you land so don't wait on a Skia roll. > > > I also noticed that Picture::Replay calls picture_->draw(), currently only > used > > by benchmarking using SkDebugCanvas, I wonder if I should also change this to > > use drawPicture? > > Hmmm. The SkDebugCanvas probably shouldn't be used for benchmarking. > > Most likely the intent is to actually unfurl the sub-picture into the > SkDebugCanvas so that all the commands can be converted to a string. Moving > forward (in a paint slimming world) this will cause problems since it will > flatten the picture hierarchy. But only if we call drawPicture(), since we call draw() in this case, we should be ok, right?
> But only if we call drawPicture(), since we call draw() in this case, we should > be ok, right? I think in the paint slimming world we will want to see the sub-pictures in the command stream (by calling SkCanvas::drawPicture) but right now they will be flattened (due to the call to SkPicture::draw). For now I would leave it alone and the need for a switch will become obvious when paint slimming lands.
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as 3174de34644c359462ce0595fc4125d76f829332
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/04cea97063df4790c91607bce25000c07119028e Cr-Commit-Position: refs/heads/master@{#296227} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
