Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
501353002:
Transfer canvas state to the next frame with noticable restrictions. (Closed)
DescriptionTransfer canvas state to the next frame with noticable restrictions.
Transfer canvas state to the next frame with the next restrictions:
- transform matrix must be invertable (to get clip area before transform)
- clip area must be rectangular
BUG=392614
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182036
Patch Set 1 : [WIP] first patch to check my approach #
Total comments: 18
Patch Set 2 : [WIP] adjusting to comments #
Total comments: 7
Patch Set 3 : Source code adjustements + [WIP] layout test problems #
Total comments: 15
Patch Set 4 : [WIP] No need to transfer canvas state in RecordingImageBufferSurface #Patch Set 5 : Release candidate #
Total comments: 25
Patch Set 6 : Second release candidate #Patch Set 7 : Release #
Messages
Total messages: 38 (6 generated)
p.sergey@samsung.com changed reviewers: + junov@chromium.org
PTAL.
Please, take a look. The idea is to transfer state using current capabilities of SKIA first. My hope here is that it will allow to cover most real-life cases quickly. Then, I guess, I will have to add SKIA feature to be able to do something like: canvas1.getStateFrom(canvas2).
This is on the right track. You need to add a layout test for this fix. Perhaps there are existing layout tests that failed before and pass with this change. You can use --additional-drt-flag=enable-display-list-2d-canvas when you call the run_webkit_tests script. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:158: // - non-square clip I think you mean non-rectangular clip https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:159: // - non-invertable clip "non-invertible" https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:165: while (!m_stateStack.empty()) { You will not have to do this once you make the stack local. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:166: state = (StateRec *)m_stateStack.back(); Please avoid C-style casts. If you switch to WTF::Deque, this will no longer be an issue. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:178: return false; This needs to be fixed before we can ship display list canvas. If you don't fix it now, please file a separate bug to it and make it a child of 386601. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:182: new (state) StateRec(); No need to do this because StateRec does not even have a constructor. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... File Source/platform/graphics/RecordingImageBufferSurface.h (right): https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.h:55: struct StateRec; This does not need to be forward declared since it is not referenced in the header. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.h:56: SkDeque m_stateStack; This does not need to be a member of RecordingImageBufferSurface. It should be a local variable in finalizeFrame(). Also, you should use a Blink class here (WTF::Deque?) https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.h:57: uint32_t m_stateStackStorage[32]; After you move this into the cpp. The 32 should be in a constant and you you add a comment to explain why 32 is a reasonable value.
On 2014/08/26 16:57:20, junov wrote: > This is on the right track. You need to add a layout test for this fix. Perhaps > there are existing layout tests that failed before and pass with this change. > You can use --additional-drt-flag=enable-display-list-2d-canvas when you call > the run_webkit_tests script. I wonder, did you try that on Linux or Android?... When I tried on Android, even without my change, 194 out of 215 tests failed... O_o :)
PTAL. Although, two questions are left: 1. StackState type and constant declarations: probably I should move them somewhere. Please, advise. 2. Layout tests, especially on Android. It might probably be worth a separate issue. Just for note: I like SkDeque more, because it seems to be more performance-efficient, then WTF::Deque.. Still, no complains, because in blink we should follow blink ideology. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:158: // - non-square clip On 2014/08/26 16:57:20, junov wrote: > I think you mean non-rectangular clip Done. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:159: // - non-invertable clip On 2014/08/26 16:57:20, junov wrote: > "non-invertible" Done. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:165: while (!m_stateStack.empty()) { On 2014/08/26 16:57:20, junov wrote: > You will not have to do this once you make the stack local. Done. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:166: state = (StateRec *)m_stateStack.back(); On 2014/08/26 16:57:20, junov wrote: > Please avoid C-style casts. If you switch to WTF::Deque, this will no longer be > an issue. Done. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:178: return false; On 2014/08/26 16:57:20, junov wrote: > This needs to be fixed before we can ship display list canvas. If you don't fix > it now, please file a separate bug to it and make it a child of 386601. I've filed crbug.com/408392. It seems I don't have any privileges to edit issues, so could you please make it a child of 386601. Or give me some privileges to be able to do it on my own. I guess I will take care of it during my next step, when I will make SKIA API to transfer canvas state. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.cpp:182: new (state) StateRec(); On 2014/08/26 16:57:20, junov wrote: > No need to do this because StateRec does not even have a constructor. I thought that I should do that in case StateRec will be changed in future, so that destructor call is required. Anyway, no need for this call with WTF::Deque. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... File Source/platform/graphics/RecordingImageBufferSurface.h (right): https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.h:55: struct StateRec; On 2014/08/26 16:57:20, junov wrote: > This does not need to be forward declared since it is not referenced in the > header. Actually, the intention was to make it inner class of RecordingImageBufferSurface. Ok, removed. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.h:56: SkDeque m_stateStack; On 2014/08/26 16:57:20, junov wrote: > This does not need to be a member of RecordingImageBufferSurface. It should be a > local variable in finalizeFrame(). Also, you should use a Blink class here > (WTF::Deque?) Done. https://codereview.chromium.org/501353002/diff/1/Source/platform/graphics/Rec... Source/platform/graphics/RecordingImageBufferSurface.h:57: uint32_t m_stateStackStorage[32]; On 2014/08/26 16:57:20, junov wrote: > After you move this into the cpp. The 32 should be in a constant and you you add > a comment to explain why 32 is a reasonable value. Done. https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics... File Source/platform/graphics/RecordingImageBufferSurface.h (left): https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.h:20: I am not sure, that this declarations belong here... Please, let me know if I should refactor it somehow.
On 2014/08/28 09:55:34, Sergey wrote: > 2. Layout tests, especially on Android. It might probably be worth a separate > issue. > Whenever you fix a bug, there should always be a test that demonstrates that the issue is fixed. For canvas rendering bugs, we mostly use Blink layout tests. If you have trouble designing a test for this or if you have any questions about this, don't hesitate to ask. This fix definitely needs a test that covers all the edge cases. The test should fail (at least partially) before applying the fix, and it should pass after.
https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics... File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.cpp:181: dstCanvas->concat(stateStack->last().m_ctm); I think this should be setMatrix rather than concat. A good layout test would confirm this. https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.cpp:187: dstCanvas->concat(stateStack->last().m_ctm); here too. https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics... File Source/platform/graphics/RecordingImageBufferSurface.h (right): https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.h:26: struct StateRec { This struct and the typedef below should be moved to the 'private' section of RecordingImageBufferSurface.
On 2014/08/28 07:52:32, Sergey wrote: > I wonder, did you try that on Linux or Android?... When I tried on Android, even > without my change, 194 out of 215 tests failed... O_o :) That sounds scary but it is actually not that bad. Most of the failure are caused by just a few actual bugs that affect many tests. There are also tests that fail due to sub-pixel differences, and just need to be baselined.
Hi. I've adjusted the source code but it seems we will have some trouble on
writing the layout test... At least, I don't have any idea how to do it now.
To illustrate what is the problem, I've uploaded to tests:
LayoutTests/canvas/state-stack.html and
LayoutTests/canvas/state-stack-pass.html. The problem is that this patch is
intended to fix canvas state transfer between canvas 2d "frames". What we
actually call "frames" for canvas 2d is the break in JS execution, which give
compositor an option to get updated contents of canvas (that is as far, as I
understand it).
But, it seems for me, that during layout test we only allowed to execute JS
once. Meaning, that if you actually try to make canvas 2d "frames", as it is
done in LayoutTests/canvas/state-stack.html, using setTimeout, it will fail.
The function, that you expect to be executed after timeout won't be executed.
At least, the test LayoutTests/canvas/state-stack.html fails, although it should
execute several times and just finish with PASS. To illustrate it I've uploaded
LayoutTests/canvas/state-stack-pass.html, which does only finishTest('PASS') on
the first JS execute and it passes.
Please, let me know if my guess regarding JS execution during layout test is
correct and if you have any idea how to overcome it... May be adjust layout
tests mechanism? :)
https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.cpp (right):
https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:181:
dstCanvas->concat(stateStack->last().m_ctm);
On 2014/08/28 14:53:00, junov wrote:
> I think this should be setMatrix rather than concat. A good layout test would
> confirm this.
Done.
https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.cpp:187:
dstCanvas->concat(stateStack->last().m_ctm);
On 2014/08/28 14:53:00, junov wrote:
> here too.
Done.
https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/RecordingImageBufferSurface.h (right):
https://codereview.chromium.org/501353002/diff/20001/Source/platform/graphics...
Source/platform/graphics/RecordingImageBufferSurface.h:26: struct StateRec {
On 2014/08/28 14:53:00, junov wrote:
> This struct and the typedef below should be moved to the 'private' section of
> RecordingImageBufferSurface.
Done.
On 2014/09/03 01:50:51, Sergey wrote: > Hi. I've adjusted the source code but it seems we will have some trouble on > writing the layout test... At least, I don't have any idea how to do it now. It seems that I've found an example I need (thanks to Jinho): LayoutTests/fast/canvas/canvas-partial-invalidation-zoomed.html. I will try to use it and will let you know the result.
jinho.bang@samsung.com changed reviewers: + jinho.bang@samsung.com
Hi Sergey, I'm not reviewer but I leave some comments. I hope my comments are helpful to you. Thank you. https://codereview.chromium.org/501353002/diff/40001/LayoutTests/canvas/state... File LayoutTests/canvas/state-stack.html (right): https://codereview.chromium.org/501353002/diff/40001/LayoutTests/canvas/state... LayoutTests/canvas/state-stack.html:2: <title>Canvas.drawImage with narrow destination.</title> nit: This should be in <head></head>. https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.cpp:138: if (!saveState(m_currentFrame->getRecordingCanvas(), &stateStack)) How does it work in the following cases? - display list mode -> fallback mode - fallback mode -> display list mode I'm not sure but I think you should apply this to m_rasterCanvas in fallBackToRasterCanvas(). @junov, what do you think? https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.cpp:144: setCurrentState(m_currentFrame->getRecordingCanvas(), &stateStack); ditto https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.cpp:163: srcCanvas->getClipBounds(&state.m_clip); I think you should use getClipDeviceBounds() instead of getClipBounds(). getClipDeviceBounds() uses SkIRect instead of SkRect. usage: Please refer to GraphicsContext::getTransformedClipBounds() https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... File Source/platform/graphics/RecordingImageBufferSurface.h (right): https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.h:24: #define INITIAL_STATE_STACK_SIZE 1 Should we use WTF::Deque? I think it is better to use WTF::LinkedStack. Also, I'm not sure if WTF::Deque<..., 1> is able to expand the internal vector. https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.h:43: public: Is this Blink coding rule? If not so, I think this is unnecessary. https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.h:45: SkRect m_clip; If you use getDeviceClipBounds(), you should probably use SkIRect.
I wrote a simple clip test.
You can use it and modify it. (I ignored callback-hell.)
Also, you should still make a test for CTM.
<!DOCTYPE html>
<html>
<head>
<script src="../../resources/js-test.js"></script>
</head>
<body>
<canvas></canvas>
<script>
if (window.testRunner) {
testRunner.dumpAsTextWithPixelResults();
testRunner.waitUntilDone();
}
var canvas = document.querySelector("canvas");
var context = canvas.getContext("2d");
requestAnimationFrame(function() {
context.save();
context.rect(0, 0, 100, 100);
context.clip();
context.fillStyle = "red";
context.fillRect(0, 0, 100, 100);
requestAnimationFrame(function() {
context.save();
context.beginPath();
context.rect(50, 50, 100, 100);
context.clip();
context.fillStyle = "green";
context.fillRect(0, 0, 100, 100);
requestAnimationFrame(function() {
context.save();
context.beginPath();
context.rect(20, 20, 50, 50);
context.clip();
context.fillStyle = "blue";
context.fillRect(0, 0, 100, 100);
requestAnimationFrame(function() {
context.save();
context.beginPath();
context.rect(60, 60, 100, 100);
context.clip();
context.fillStyle = "black";
context.fillRect(0, 0, 100, 100);
if (window.testRunner)
testRunner.notifyDone();
});
});
});
});
</script>
</body>
</html>
On 2014/09/03 06:59:32, Sergey wrote: > On 2014/09/03 01:50:51, Sergey wrote: > > Hi. I've adjusted the source code but it seems we will have some trouble on > > writing the layout test... At least, I don't have any idea how to do it now. > > It seems that I've found an example I need (thanks to Jinho): > LayoutTests/fast/canvas/canvas-partial-invalidation-zoomed.html. > I will try to use it and will let you know the result. Basically, you need to create an asynchronous test by using testRunner.waitUntilDone() and testRuner.notifyDone(). Then you are free to use requestAnimationFrame to produce a sequence of frames. As long as you eventually call notifyDone(), otherwise the test will time out.
https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.cpp:163: srcCanvas->getClipBounds(&state.m_clip); On 2014/09/03 10:50:17, zino wrote: > I think you should use getClipDeviceBounds() instead of getClipBounds(). > getClipDeviceBounds() uses SkIRect instead of SkRect. > > usage: Please refer to GraphicsContext::getTransformedClipBounds() This is a good point. Using getDeviceClipBounds saves a matrix inversion. At the other end, in setCurrentState you will have to apply the clip in device space though. You can do that by setting ctm to identity, set the clip, then setMatrix.
On 2014/09/03 15:16:38, junov wrote: > Basically, you need to create an asynchronous test by using > testRunner.waitUntilDone() and testRuner.notifyDone(). Then you are free to use > requestAnimationFrame to produce a sequence of frames. As long as you > eventually call notifyDone(), otherwise the test will time out. It seems that I am making a progress in this direction, but now I am stuck because I can't think of a test, that would fail with fallback and would not fail with the new state stack transfer. The point is that previously, if we had any canvas state stack, we fell back. During drawing to raster canvas all the clip and transform operations were passed to raster canvas, so we had no examples of incorrect stack transfer. If the above is true, then I can only make a layout test that will ensure that correct state stack transfer is not broken. Please, correct me if I am wrong.
https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.cpp:138: if (!saveState(m_currentFrame->getRecordingCanvas(), &stateStack)) On 2014/09/03 10:50:17, zino wrote: > How does it work in the following cases? > - display list mode -> fallback mode > - fallback mode -> display list mode > > I'm not sure but I think you should apply this to m_rasterCanvas in > fallBackToRasterCanvas(). > > @junov, what do you think? First, right now there is no "fallback mode -> display list mode" option. Second, as I understand it, transferring state of the canvas during "display list mode -> fallback mode" is done, when we replay all the operations from recording canvas into raster canvas. See function RecordingImageBufferSurface::fallBackToRasterCanvas, lines 67 and 72. @Junov, please, correct me, if I am wrong here. https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.cpp:144: setCurrentState(m_currentFrame->getRecordingCanvas(), &stateStack); On 2014/09/03 10:50:17, zino wrote: > ditto It is not required at this point, as we are not going to fallback from here. The idea is to identify our ability to transfer state at your previous comment point and return from this function, saying that we were unable to finalize frame. This will trigger a fallback to rasterizing canvas on main thread. https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... File Source/platform/graphics/RecordingImageBufferSurface.h (right): https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.h:24: #define INITIAL_STATE_STACK_SIZE 1 On 2014/09/03 10:50:17, zino wrote: > Should we use WTF::Deque? I think it is better to use WTF::LinkedStack. > Also, I'm not sure if WTF::Deque<..., 1> is able to expand the internal vector. Junov, what do you think? I would probably agree with Jinho, since LinkedStack does not move objects, when it has to allocate more memory. And it seems Deque is mostly design for big capacities, while I don't expect that anyone would write a web page with more then 5-10 canvas state saves...
On 2014/09/04 10:26:18, Sergey wrote: > On 2014/09/03 15:16:38, junov wrote: > > Basically, you need to create an asynchronous test by using > > testRunner.waitUntilDone() and testRuner.notifyDone(). Then you are free to > use > > requestAnimationFrame to produce a sequence of frames. As long as you > > eventually call notifyDone(), otherwise the test will time out. > > It seems that I am making a progress in this direction, but now I am stuck > because I can't think of a test, that would fail with fallback and would not > fail with the new state stack transfer. The point is that previously, if we had > any canvas state stack, we fell back. During drawing to raster canvas all the > clip and transform operations were passed to raster canvas, so we had no > examples of incorrect stack transfer. > > If the above is true, then I can only make a layout test that will ensure that > correct state stack transfer is not broken. > > Please, correct me if I am wrong. In that case you should just write a test that would fail if we did not have any mechanism to transfer state, even if the test passes before your change (thanks to the fallback). To be honest I probably should have written such tests when I implemented the fallback.
On 2014/09/04 10:26:51, Sergey wrote: > https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... > File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): > > https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... > Source/platform/graphics/RecordingImageBufferSurface.cpp:138: if > (!saveState(m_currentFrame->getRecordingCanvas(), &stateStack)) > On 2014/09/03 10:50:17, zino wrote: > > How does it work in the following cases? > > - display list mode -> fallback mode > > - fallback mode -> display list mode > > > > I'm not sure but I think you should apply this to m_rasterCanvas in > > fallBackToRasterCanvas(). > > > > @junov, what do you think? > > First, right now there is no "fallback mode -> display list mode" option. > > Second, as I understand it, transferring state of the canvas during "display > list mode -> fallback mode" is done, when we replay all the operations from > recording canvas into raster canvas. See function > RecordingImageBufferSurface::fallBackToRasterCanvas, lines 67 and 72. > @Junov, please, correct me, if I am wrong here. That is correct, except that I believe the playback into raster canvas will automatically balance save/restore calls (needs to be verified), so I think the fallback mechanism may be discarding state right now. But we can treat that as a separate bug and fix it in a follow-up change. > > https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... > Source/platform/graphics/RecordingImageBufferSurface.cpp:144: > setCurrentState(m_currentFrame->getRecordingCanvas(), &stateStack); > On 2014/09/03 10:50:17, zino wrote: > > ditto > > It is not required at this point, as we are not going to fallback from here. The > idea is to identify our ability to transfer state at your previous comment point > and return from this function, saying that we were unable to finalize frame. > This will trigger a fallback to rasterizing canvas on main thread. > > https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... > File Source/platform/graphics/RecordingImageBufferSurface.h (right): > > https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... > Source/platform/graphics/RecordingImageBufferSurface.h:24: #define > INITIAL_STATE_STACK_SIZE 1 > On 2014/09/03 10:50:17, zino wrote: > > Should we use WTF::Deque? I think it is better to use WTF::LinkedStack. > > Also, I'm not sure if WTF::Deque<..., 1> is able to expand the internal > vector. > > Junov, what do you think? I would probably agree with Jinho, since LinkedStack > does not move objects, when it has to allocate more memory. And it seems Deque > is mostly design for big capacities, while I don't expect that anyone would > write a web page with more then 5-10 canvas state saves... Yes, I agree LinkedStack is a better fit.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
First of all, sorry for delay, we had some holidays here in Korea. On 2014/09/04 13:43:18, junov wrote: > On 2014/09/04 10:26:51, Sergey wrote: > > > https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... > > File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): > > > > > https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... > > Source/platform/graphics/RecordingImageBufferSurface.cpp:138: if > > (!saveState(m_currentFrame->getRecordingCanvas(), &stateStack)) > > On 2014/09/03 10:50:17, zino wrote: > > > How does it work in the following cases? > > > - display list mode -> fallback mode > > > - fallback mode -> display list mode > > > > > > I'm not sure but I think you should apply this to m_rasterCanvas in > > > fallBackToRasterCanvas(). > > > > > > @junov, what do you think? > > > > First, right now there is no "fallback mode -> display list mode" option. > > > > Second, as I understand it, transferring state of the canvas during "display > > list mode -> fallback mode" is done, when we replay all the operations from > > recording canvas into raster canvas. See function > > RecordingImageBufferSurface::fallBackToRasterCanvas, lines 67 and 72. > > @Junov, please, correct me, if I am wrong here. > > That is correct, except that I believe the playback into raster canvas will > automatically balance save/restore calls (needs to be verified), so I think the > fallback mechanism may be discarding state right now. But we can treat that as a > separate bug and fix it in a follow-up change. Actually, while I was trying to make a layout test that would fail without correct canvas state transfer, I found out that I can't make it fail, even if I comment state transfer source code. Please, look into my latest patch with the layout test. The test will fail, if you will comment one or two of context.restore() calls. But it doesn't fail even with the modifications, done to the RecordingImageBufferSurface right now. My guess of the reason is that GraphicsContext (m_imageBuffer->context()) actually saves current canvas 2d state stack (see GraphicsContext::m_paintStateStack) and somehow uses it with the canvas, that is passed to it with m_imageBuffer->context()->resetCanvas(...); How do you think, Junov, am I correct here? If yes, I will write one more layout test with more complex canvas 2d state stack and hopefully we will be done with this issue. > > Junov, what do you think? I would probably agree with Jinho, since LinkedStack > > does not move objects, when it has to allocate more memory. And it seems Deque > > is mostly design for big capacities, while I don't expect that anyone would > > write a web page with more then 5-10 canvas state saves... > > Yes, I agree LinkedStack is a better fit. We will use it, if my above guess is not correct..
https://codereview.chromium.org/501353002/diff/40001/LayoutTests/canvas/state... File LayoutTests/canvas/state-stack.html (right): https://codereview.chromium.org/501353002/diff/40001/LayoutTests/canvas/state... LayoutTests/canvas/state-stack.html:2: <title>Canvas.drawImage with narrow destination.</title> On 2014/09/03 10:50:17, zino wrote: > nit: This should be in <head></head>. Layout test re-written. https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.cpp:163: srcCanvas->getClipBounds(&state.m_clip); On 2014/09/03 15:24:39, junov wrote: > On 2014/09/03 10:50:17, zino wrote: > > I think you should use getClipDeviceBounds() instead of getClipBounds(). > > getClipDeviceBounds() uses SkIRect instead of SkRect. > > > > usage: Please refer to GraphicsContext::getTransformedClipBounds() > > This is a good point. Using getDeviceClipBounds saves a matrix inversion. At the > other end, in setCurrentState you will have to apply the clip in device space > though. You can do that by setting ctm to identity, set the clip, then > setMatrix. Probably not required here at all. https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... File Source/platform/graphics/RecordingImageBufferSurface.h (right): https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.h:43: public: On 2014/09/03 10:50:17, zino wrote: > Is this Blink coding rule? > If not so, I think this is unnecessary. Probably not required here at all. https://codereview.chromium.org/501353002/diff/40001/Source/platform/graphics... Source/platform/graphics/RecordingImageBufferSurface.h:45: SkRect m_clip; On 2014/09/03 10:50:18, zino wrote: > If you use getDeviceClipBounds(), you should probably use SkIRect. Probably not required here at all.
On 2014/09/11 12:58:40, Sergey wrote: > > > My guess of the reason is that GraphicsContext (m_imageBuffer->context()) > actually saves current canvas 2d state stack (see > GraphicsContext::m_paintStateStack) and somehow uses it with the canvas, that is > passed to it with m_imageBuffer->context()->resetCanvas(...); > > How do you think, Junov, am I correct here? If yes, I will write one more layout > test with more complex canvas 2d state stack and hopefully we will be done with > this issue. > Not quite. The clip and matrix is stored in SkCanvas, m_paintStateStack stores the rest of the state. The two are complementary.
Here is an example of a test that fails currently with display list 2d canvas
enabled:
<!DOCTYPE html>
<html><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<script>
var ctx;
function drawSecondFrame() {
ctx.restore();
ctx.fillStyle = 'green';
ctx.fillRect(25, 25, 50, 50);
if (window.testRunner) {
testRunner.notifyDone();
}
}
window.onload = function() {
if (window.testRunner) {
testRunner.waitUntilDone();
}
ctx = document.getElementById('c').getContext('2d');
ctx.fillStyle = 'red';
ctx.fillRect(50, 50, 100, 100);
ctx.scale(2, 2);
ctx.save()
ctx.scale(0.5, 0.5);
window.requestAnimationFrame(drawSecondFrame);
}
</script>
</head>
<body>
<p>This test should draw a 100px-by-100px green square no red should be
visible.</p>
<canvas id="c" width="200" height="200">
</canvas>
</body></html>
Patchset #5 (id:140001) has been deleted
On 2014/09/11 13:55:49, junov wrote: > Here is an example of a test that fails currently with display list 2d canvas > enabled: Thanks, this has shown me my mistake: in my test new SkPictureRecorder was not created for the next frame. Please, take a look at the new patch. I hope that I've managed to follow all the previous notes.
lgtm with minor corrections. https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/canvas-state-stack-simple.html (right): https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:21: context.rect(0,0,300,300); This test would be more interesting if you used a small rect here, or even an empty rect. https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... Source/platform/graphics/RecordingImageBufferSurface.cpp:155: // - non-invertible clip I think you mean "inverted clip"?
Dear Sergey, Looks good to me! But could you please follow layout-test-style-guidelines? http://www.chromium.org/blink/coding-style/layout-test-style-guidelines https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/canvas-state-stack-simple.html (right): https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:3: <head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> These tags are normally omitted. <html>, <head>, <body> http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h... I think your new test should be the following form. -------------------------------------------- <!DOCTYPE html> <canvas></canvas> <script> var context; if (window.testRunner) ... context... context... requestAnimationFrame(drawSecondFrame); function drawSecondFrame() { ... } </script> -------------------------------------------- https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:9: testRunner.waitUntilDone(); fix indentation? Either of 2 space and 4 space indentation. The usage should be consistent within a test file. http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-I... https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:10: context = document.getElementById("c").getContext("2d"); "c" -> 'c' "2d" -> '2d' Some tests use double quote for HTML (e.g. <script src="some.js">) and single quote for JavaScript (e.g. var image = 'resources/image.jpg';), but it could be either. The usage should be consistent within a test file. http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-Q... https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:11: context.scale(2,2); fix spacing? https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:21: context.rect(0,0,300,300); On 2014/09/12 14:54:35, junov wrote: > This test would be more interesting if you used a small rect here, or even an > empty rect. good point. https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:22: context.clip(); Also, I think it is better to add context.fillRect() here. https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:38: <canvas id="c" width=300 height=300></canvas> width="300" height="300" https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... Source/platform/graphics/RecordingImageBufferSurface.cpp:153: // FIXME(crbug.com/392614): handle transferring complex state from the current picture to the new one: I think you should avoid FIXME(attribution) in Blink. http://www.chromium.org/blink/coding-style#TOC-Comments (3) // FIXME: handle transferring complex state from the current picture to the new one. // Please see http://crbug.com/392614 https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... Source/platform/graphics/RecordingImageBufferSurface.cpp:164: // FIXME(crbug.com/408392): don't mess up the state in case we will have to fallback ditto https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... Source/platform/graphics/RecordingImageBufferSurface.cpp:172: // FIXME(crbug.com/408392): don't mess up the state in case we will have to fallback ditto
PTAL. https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/canvas-state-stack-simple.html (right): https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:3: <head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> On 2014/09/13 16:00:28, zino wrote: > These tags are normally omitted. > <html>, <head>, <body> > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h... This is probably a recommendation, when you are going to test something with JS only and use 'shouldBe' checks e.t.c. In my case I would probably prefer to emphasize that resulted html is compared. And, actually, it seems to be the same case for many other layout tests in the current folder: LayoutTests/fast/canvas/canvas-skia-excessive-size.html LayoutTests/fast/canvas/canvas-shadow.html LayoutTests/fast/canvas/canvas-setTransform.html LayoutTests/fast/canvas/canvas-scale-shadowBlur.html LayoutTests/fast/canvas/canvas-partial-invalidation-zoomed.html https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:9: testRunner.waitUntilDone(); On 2014/09/13 16:00:28, zino wrote: > fix indentation? > > Either of 2 space and 4 space indentation. > The usage should be consistent within a test file. > > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-I... Done. https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:10: context = document.getElementById("c").getContext("2d"); On 2014/09/13 16:00:28, zino wrote: > "c" -> 'c' > "2d" -> '2d' > > Some tests use double quote for HTML (e.g. <script src="some.js">) and single > quote for JavaScript (e.g. var image = 'resources/image.jpg';), but it could be > either. > The usage should be consistent within a test file. > > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-Q... Done. https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:11: context.scale(2,2); On 2014/09/13 16:00:28, zino wrote: > fix spacing? Here everything seems fine... Please, explain. https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:21: context.rect(0,0,300,300); On 2014/09/12 14:54:35, junov wrote: > This test would be more interesting if you used a small rect here, or even an > empty rect. Done. https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:22: context.clip(); On 2014/09/13 16:00:28, zino wrote: > Also, I think it is better to add context.fillRect() here. May be that would be excessive, wouldn't it? https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:38: <canvas id="c" width=300 height=300></canvas> On 2014/09/13 16:00:28, zino wrote: > width="300" height="300" Done. https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... File Source/platform/graphics/RecordingImageBufferSurface.cpp (right): https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... Source/platform/graphics/RecordingImageBufferSurface.cpp:153: // FIXME(crbug.com/392614): handle transferring complex state from the current picture to the new one: On 2014/09/13 16:00:28, zino wrote: > I think you should avoid FIXME(attribution) in Blink. > http://www.chromium.org/blink/coding-style#TOC-Comments (3) > > // FIXME: handle transferring complex state from the current picture to the new > one. > // Please see http://crbug.com/392614 I think one line comment would be better... PTAL. https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... Source/platform/graphics/RecordingImageBufferSurface.cpp:155: // - non-invertible clip On 2014/09/12 14:54:35, junov wrote: > I think you mean "inverted clip"? I meant situation, when we can't invert transform matrix to get local clip bounds (see SkCanvas::getClipBounds). This is obsolete since the start of SkCanvas::getClipDeviceBounds usage in this code, so, removed it. https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... Source/platform/graphics/RecordingImageBufferSurface.cpp:164: // FIXME(crbug.com/408392): don't mess up the state in case we will have to fallback On 2014/09/13 16:00:28, zino wrote: > ditto Done. https://codereview.chromium.org/501353002/diff/160001/Source/platform/graphic... Source/platform/graphics/RecordingImageBufferSurface.cpp:172: // FIXME(crbug.com/408392): don't mess up the state in case we will have to fallback On 2014/09/13 16:00:28, zino wrote: > ditto Done.
I'm sorry for interrupting landing this CL with layout-style-guideline. Thank you and non-owner lgtm. https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... File LayoutTests/fast/canvas/canvas-state-stack-simple.html (right): https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:3: <head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> On 2014/09/15 02:45:35, Sergey wrote: > On 2014/09/13 16:00:28, zino wrote: > > These tags are normally omitted. > > <html>, <head>, <body> > > > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h... > > This is probably a recommendation, when you are going to test something with JS > only and use 'shouldBe' checks e.t.c. > > In my case I would probably prefer to emphasize that resulted html is compared. > And, actually, it seems to be the same case for many other layout tests in the > current folder: > LayoutTests/fast/canvas/canvas-skia-excessive-size.html > LayoutTests/fast/canvas/canvas-shadow.html > LayoutTests/fast/canvas/canvas-setTransform.html > LayoutTests/fast/canvas/canvas-scale-shadowBlur.html > LayoutTests/fast/canvas/canvas-partial-invalidation-zoomed.html I don't think the rule is related to "js-test" framework. I'm not sure but I think the guide is always applied if your test isn't w3c platform test. http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-S... However, It is not important. (just nits) So, I'm fine anyway. https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... LayoutTests/fast/canvas/canvas-state-stack-simple.html:11: context.scale(2,2); On 2014/09/15 02:45:36, Sergey wrote: > On 2014/09/13 16:00:28, zino wrote: > > fix spacing? > > Here everything seems fine... Please, explain. It is not important but it is better to have a consistency. The [space] character should be consistent within a test file. context.scale(2,2); -> context.scale(2,[space]2); context.rect(0,0,75,75); -> context.rect(0,[space]0,[space]75,[space]75); context.scale(0.5,0.5); -> .. context.rect(0,0,300,300); -> .. You already use [space] character here. context.fillRect(0, 0, 30, 30); context.fillRect(50, 50, 30, 30); context.fillRect(0, 0, 30, 30); context.fillRect(50, 50, 30, 30);
Thank for the input zino. re-lgtm
The CQ bit was checked by p.sergey@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/501353002/200001
On 2014/09/15 09:51:03, zino wrote: > I'm sorry for interrupting landing this CL with layout-style-guideline. > Thank you and non-owner lgtm. > > I don't think the rule is related to "js-test" framework. > I'm not sure but I think the guide is always applied if your test isn't w3c > platform test. > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-S... Ok, I get it. Small change, so, fixed. > However, It is not important. (just nits) So, I'm fine anyway. > > https://codereview.chromium.org/501353002/diff/160001/LayoutTests/fast/canvas... > LayoutTests/fast/canvas/canvas-state-stack-simple.html:11: context.scale(2,2); > On 2014/09/15 02:45:36, Sergey wrote: > > On 2014/09/13 16:00:28, zino wrote: > > > fix spacing? > > > > Here everything seems fine... Please, explain. > > It is not important but it is better to have a consistency. > The [space] character should be consistent within a test file. > > context.scale(2,2); -> context.scale(2,[space]2); > context.rect(0,0,75,75); -> context.rect(0,[space]0,[space]75,[space]75); > context.scale(0.5,0.5); -> .. > context.rect(0,0,300,300); -> .. > > You already use [space] character here. > context.fillRect(0, 0, 30, 30); > context.fillRect(50, 50, 30, 30); > context.fillRect(0, 0, 30, 30); > context.fillRect(50, 50, 30, 30); This is now also understandable. Also fixed. Thank you for review.
Message was sent while issue was closed.
Committed patchset #7 (id:200001) as 182036 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
