GeneratedImage::drawPattern() misuses SkPictureBuilder |
|||||
Issue descriptionGeneratedImage::drawPattern() records a pattern SkPicture using SkPictureBuilder. SkPictureBuilder builder(tileRect, nullptr, &destContext); builder.context().beginRecording(tileRect); drawTile(builder.context(), srcRect); sk_sp<SkPicture> tilePicture = builder.endRecording(); However this is a misuse. SkPictureBuilder expects content to be drawn in a DrawingRecorder. Instead the code directly calls GraphicsContext::beginRecording() and draws into it. builder.context().beginRecording(tileRect); drawTile(builder.context(), srcRect); In turn builder.endRecording() also calls GraphicsContext::beginRecording() and plays back any recorded display list items (of which there are none). Due to undefined Skia SkPictureRecorded::beginRecording() behavior the drawTile content still ends up in the right SkPicture. We should fix this, and make SkPictureRecorded::beginRecording() assert in this case.
,
Dec 8 2016
I'm unsure what's the right solution here. - Create a DrawingRecorder? What would be the DisplayItemClient? - Don't use SkPictureBuilder? - Change SkPictureBuilder to allow non display-list recording mode?
,
Dec 8 2016
Can we just use destContext to record tilePicture? If yes, I would prefer not using SkPictureBuilder, otherwise would prefer creating a DrawingRecorder using the GeneratedImage or a temp dummy object as the DisplayItemClient.
,
Dec 9 2016
#3: We can't use destContext as that's recording the outer picture, but we can make a new GraphicsContext. patch up https://codereview.chromium.org/2565433003/
,
Dec 9 2016
,
Dec 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/45c66caf87f28914324b9851f83216fa58971915 commit 45c66caf87f28914324b9851f83216fa58971915 Author: vmiura <vmiura@chromium.org> Date: Fri Dec 09 05:36:46 2016 Fix GeneratedImage::drawPattern() use of SkPictureBuilder. SkPictureBuilder records DisplayItems and plays them into an SkPicture. GeneratedImage::drawPattern() was misusing SkPictureBuilder; using it to create a GraphicsContext and draw to it directly. It results in the pattern: 1) GraphicsContext::beginRecording() 2) Draw content to GraphicsContext 3) GraphicsContext::beginRecording() again 5) SkPicture picture = GraphicsContext::endRecording() This is hitting undefined behavior in SkPictureRecorder which happens to work but is a bug. This change removes SkPictureBuilder and records directly to a GraphicsContext. BUG=672618 Review-Url: https://codereview.chromium.org/2565433003 Cr-Commit-Position: refs/heads/master@{#437478} [modify] https://crrev.com/45c66caf87f28914324b9851f83216fa58971915/third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp [modify] https://crrev.com/45c66caf87f28914324b9851f83216fa58971915/third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp
,
Dec 9 2016
,
Dec 9 2016
PlaceholderImage::imageForCurrentFrame() needs the same fix.
,
Dec 12 2016
I think BoxReflectionUtils (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/BoxReflectionUtils.cpp?rcl=0&l=58) needs a similar solution (though it currently doesn't have the beginRecording issue). Actually it's using what I suggested #3 using a DrawingRecorder, but now I feel it's unnecessarily verbose, and prefer the method used in vmiura's r437478. Perhaps it would be better if - inherit settings (context disabled, device scale, etc.) from the input GraphicsContext to the local GraphicsContext; - reuse a dummy PaintController (or make it optional for GraphicsContext again?) which actually does nothing in such cases. What about the following code? GraphicsContext context(destContext); context.beginRecording(); ... drawing code ... sk_sp<SkPicture> picture = context.endRecording();
,
Dec 13 2016
#9: That pattern sgtm.
,
Jul 30
Is this still an issue? |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by reed@google.com
, Dec 8 2016