canvas: calling toDataURL without context doesn't create a buffer of canvas 2d.

canvas: calling toDataURL without context doesn't create a buffer of canvas 2d.

Currently, calling toDataURL without context creates a software buffer for canvas 2d.
After it, we can create a webgl context, but canvas implementation reuses the software
buffer for webgl. It works well because webgl has software fallback code but it causes
unnecessary slow performance when copying webgl contents to other webgl or canvas 2d.

This CL doesn't make a buffer when a context doesn't exist because we don't know which
context will be created.

TEST=fast/canvas/webgl/draw-webgl-to-canvas-2d-after-to-data-url-without-context.html
BUG=331181
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185933

[dshwang, 2014-11-22 19:30:55]

dongseong.hwang@intel.com changed reviewers:
+ junov@chromium.org

[dshwang, 2014-11-22 19:30:56]

could you review this small cl?

[Justin Novosad, 2014-11-24 17:20:57]

Not lgtm.

The current behavior complies with the specification.  The spec says: If the
canvas has no pixels (i.e. either its horizontal dimension or its vertical
dimension is zero) then the method must return the string "data:,". (This is the
shortest data: URL; it represents the empty string in a text/plain resource.)  

Also, my interpretation is that if the canvas has a non-empty size but no
context has been created, to DataURL should encode an image that contains all
transparent black pixels.

If you disagree with the current behavior, you should make a proposal to change
the spec on the whatwg mailing list. But beware that breaking backwards
compatibility--which I think this change does--is not taken lightly.

[dshwang, 2014-11-24 17:38:18]

On 2014/11/24 17:20:57, junov wrote:
> Not lgtm.
> 
> The current behavior complies with the specification.  The spec says: If the
> canvas has no pixels (i.e. either its horizontal dimension or its vertical
> dimension is zero) then the method must return the string "data:,". (This is
the
> shortest data: URL; it represents the empty string in a text/plain resource.) 

> 
> Also, my interpretation is that if the canvas has a non-empty size but no
> context has been created, to DataURL should encode an image that contains all
> transparent black pixels.
> 
> If you disagree with the current behavior, you should make a proposal to
change
> the spec on the whatwg mailing list. But beware that breaking backwards
> compatibility--which I think this change does--is not taken lightly.

I agree on you. We should preserve the existing behavior.
For example, MDN HTMLCanvasElement.toDataURL show the following example,
https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement.toDataURL

<canvas id="canvas" width="5" height="5"></canvas>
var canvas = document.getElementById("canvas");
var dataURL = canvas.toDataURL();
console.log(dataURL);
// "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNby
// blAAAADElEQVQImWNgoBMAAABpAAFEI8ARAAAAAElFTkSuQmCC"

[dshwang, 2014-11-24 17:43:32]

dongseong.hwang@intel.com changed reviewers:
+ kbr@chromium.org

[dshwang, 2014-11-24 17:44:08]

junov, I change this CL's goal. Please read new description of this CL.
junov, kbr, could you review?

[Justin Novosad, 2014-11-24 18:06:02]

On 2014/11/24 17:44:08, dshwang wrote:
> junov, I change this CL's goal. Please read new description of this CL.
> junov, kbr, could you review?

This makes a lot more sense.  Please add a layout test that shows the bug that
happens when calling toDataURL before and after creating a WebGL context.

[dshwang, 2014-11-24 18:45:41]

On 2014/11/24 18:06:02, junov wrote:
> On 2014/11/24 17:44:08, dshwang wrote:
> > junov, I change this CL's goal. Please read new description of this CL.
> > junov, kbr, could you review?
> 
> This makes a lot more sense.  Please add a layout test that shows the bug that
> happens when calling toDataURL before and after creating a WebGL context.

Good point. I tried, but failed to make good layout test.
As I mentioned in description, webgl works well with software buffer also.
For example, see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
WebGL is designed to get along with unaccelerated image buffer.

[dshwang, 2014-11-24 19:07:20]

On 2014/11/24 18:45:41, dshwang wrote:
> On 2014/11/24 18:06:02, junov wrote:
> > On 2014/11/24 17:44:08, dshwang wrote:
> > > junov, I change this CL's goal. Please read new description of this CL.
> > > junov, kbr, could you review?
> > 
> > This makes a lot more sense.  Please add a layout test that shows the bug
that
> > happens when calling toDataURL before and after creating a WebGL context.
> 
> Good point. I tried, but failed to make good layout test.
> As I mentioned in description, webgl works well with software buffer also.
> For example, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> WebGL is designed to get along with unaccelerated image buffer.

I add new test,
fast/canvas/webgl/draw-webgl-to-canvas-2d-after-to-data-url-without-context.html
though it is passed before this change.
The test prevents future regression.

[Justin Novosad, 2014-11-24 19:35:22]

From your description, I  was under the impression that there was a bug if you
did something like this:

<img id='result'>
<script>
window.onload = function () {
    var canvas = document.createElement('canvas');
    canvas.toDataURL();
    ctx = canvas.getContext('webgl');
    (... draw something into the canvas ...)
    var resultImage = document.getElementById('result');
    if (window.testRunner) {
        testRunner.waitUntilDone();
        resultImage.onload = function() { testRunner.notifyDone(); }
    }
    resultImage.src = canvas.toDataURL();
}
</script>

My impression was that the second call to toDataURL would return a blank image
(from the software buffer that was created during the first call to toDataURL). 
Did I misunderstand your explanation?

[dshwang, 2014-11-24 19:41:52]

On 2014/11/24 19:35:22, junov wrote:
> From your description, I  was under the impression that there was a bug if you
> did something like this:
> 
> <img id='result'>
> <script>
> window.onload = function () {
>     var canvas = document.createElement('canvas');
>     canvas.toDataURL();
>     ctx = canvas.getContext('webgl');
>     (... draw something into the canvas ...)
>     var resultImage = document.getElementById('result');
>     if (window.testRunner) {
>         testRunner.waitUntilDone();
>         resultImage.onload = function() { testRunner.notifyDone(); }
>     }
>     resultImage.src = canvas.toDataURL();
> }
> </script>
> 
> My impression was that the second call to toDataURL would return a blank image
> (from the software buffer that was created during the first call to
toDataURL). 
> Did I misunderstand your explanation?

second toDaraURL call is fine, because WebGL can readback gpu texture into the
software buffer.
WebGL get along with software buffer because css -webkit-canvas requires WebGL
to handle software buffer.

[Justin Novosad, 2014-11-24 19:54:06]

On 2014/11/24 19:41:52, dshwang wrote:
> On 2014/11/24 19:35:22, junov wrote:
> > From your description, I  was under the impression that there was a bug if
you
> > did something like this:
> > 
> > <img id='result'>
> > <script>
> > window.onload = function () {
> >     var canvas = document.createElement('canvas');
> >     canvas.toDataURL();
> >     ctx = canvas.getContext('webgl');
> >     (... draw something into the canvas ...)
> >     var resultImage = document.getElementById('result');
> >     if (window.testRunner) {
> >         testRunner.waitUntilDone();
> >         resultImage.onload = function() { testRunner.notifyDone(); }
> >     }
> >     resultImage.src = canvas.toDataURL();
> > }
> > </script>
> > 
> > My impression was that the second call to toDataURL would return a blank
image
> > (from the software buffer that was created during the first call to
> toDataURL). 
> > Did I misunderstand your explanation?
> 
> second toDaraURL call is fine, because WebGL can readback gpu texture into the
> software buffer.
> WebGL get along with software buffer because css -webkit-canvas requires WebGL
> to handle software buffer.

I see, so the problem was strictly with performance?

[dshwang, 2014-11-24 20:08:37]

On 2014/11/24 19:54:06, junov wrote:
> On 2014/11/24 19:41:52, dshwang wrote:
> > On 2014/11/24 19:35:22, junov wrote:
> > > From your description, I  was under the impression that there was a bug if
> you
> > > did something like this:
> > > 
> > > <img id='result'>
> > > <script>
> > > window.onload = function () {
> > >     var canvas = document.createElement('canvas');
> > >     canvas.toDataURL();
> > >     ctx = canvas.getContext('webgl');
> > >     (... draw something into the canvas ...)
> > >     var resultImage = document.getElementById('result');
> > >     if (window.testRunner) {
> > >         testRunner.waitUntilDone();
> > >         resultImage.onload = function() { testRunner.notifyDone(); }
> > >     }
> > >     resultImage.src = canvas.toDataURL();
> > > }
> > > </script>
> > > 
> > > My impression was that the second call to toDataURL would return a blank
> image
> > > (from the software buffer that was created during the first call to
> > toDataURL). 
> > > Did I misunderstand your explanation?
> > 
> > second toDaraURL call is fine, because WebGL can readback gpu texture into
the
> > software buffer.
> > WebGL get along with software buffer because css -webkit-canvas requires
WebGL
> > to handle software buffer.
> 
> I see, so the problem was strictly with performance?

Yes, the problem is only performance

[Justin Novosad, 2014-11-24 20:17:56]

https://codereview.chromium.org/750273003/diff/60001/LayoutTests/fast/canvas/...
File
LayoutTests/fast/canvas/webgl/draw-webgl-to-canvas-2d-after-to-data-url-without-context.html
(right):

https://codereview.chromium.org/750273003/diff/60001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/webgl/draw-webgl-to-canvas-2d-after-to-data-url-without-context.html:3:
<body>
Please add some text here to describe what this test does.

[dshwang, 2014-11-24 20:27:00]

On 2014/11/24 20:17:56, junov wrote:
>
https://codereview.chromium.org/750273003/diff/60001/LayoutTests/fast/canvas/...
> File
>
LayoutTests/fast/canvas/webgl/draw-webgl-to-canvas-2d-after-to-data-url-without-context.html
> (right):
> 
>
https://codereview.chromium.org/750273003/diff/60001/LayoutTests/fast/canvas/...
>
LayoutTests/fast/canvas/webgl/draw-webgl-to-canvas-2d-after-to-data-url-without-context.html:3:
> <body>
> Please add some text here to describe what this test does.

Yes. Done.

[Justin Novosad, 2014-11-24 22:27:57]

lgtm with typo fixed

https://codereview.chromium.org/750273003/diff/80001/LayoutTests/fast/canvas/...
File
LayoutTests/fast/canvas/webgl/draw-webgl-to-canvas-2d-after-to-data-url-without-context.html
(right):

https://codereview.chromium.org/750273003/diff/80001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/webgl/draw-webgl-to-canvas-2d-after-to-data-url-without-context.html:6:
reduntant toDataURL().
"redundant"

[Ken Russell (switch to Gerrit), 2014-11-25 01:13:13]

LGTM with Justin's concerns addressed.

[dshwang, 2014-11-25 08:15:32]

Patchset #6 (id:100001) has been deleted

[dshwang, 2014-11-25 08:17:47]

On 2014/11/25 01:13:13, Ken Russell wrote:
> LGTM with Justin's concerns addressed.

On 2014/11/24 22:27:57, junov wrote:
> lgtm with typo fixed

Thank you for review!

> 
>
https://codereview.chromium.org/750273003/diff/80001/LayoutTests/fast/canvas/...
> File
>
LayoutTests/fast/canvas/webgl/draw-webgl-to-canvas-2d-after-to-data-url-without-context.html
> (right):
> 
>
https://codereview.chromium.org/750273003/diff/80001/LayoutTests/fast/canvas/...
>
LayoutTests/fast/canvas/webgl/draw-webgl-to-canvas-2d-after-to-data-url-without-context.html:6:
> reduntant toDataURL().
> "redundant"

Oops, done.

[dshwang, 2014-11-25 08:17:53]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-11-25 08:18:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/750273003/120001

[commit-bot: I haz the power, 2014-11-25 09:46:20]

Committed patchset #6 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185933

[Noel Gordon, 2015-01-06 10:43:58]

noel@chromium.org changed reviewers:
+ noel@chromium.org

[Noel Gordon, 2015-01-06 10:43:58]

https://codereview.chromium.org/750273003/diff/120001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/750273003/diff/120001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:418: RefPtrWillBeRawPtr<ImageData>
imageData = ImageData::create(m_size);
/me poking around here because ...

Cluster fuzz produced a complaint about toDataURL after this patch: did you guys
get bug mail at all?

Drive-by comment: If a user has never called getContext("2d") or
getContext("3d") on their canvas (m_context has not been created), why should
toDataURL even attempt to return an image, which the code seems to be doing
here? What does Mozilla do? /me curious.

[dshwang, 2015-01-06 11:55:56]

https://codereview.chromium.org/750273003/diff/120001/Source/core/html/HTMLCa...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/750273003/diff/120001/Source/core/html/HTMLCa...
Source/core/html/HTMLCanvasElement.cpp:418: RefPtrWillBeRawPtr<ImageData>
imageData = ImageData::create(m_size);
On 2015/01/06 10:43:58, noel gordon wrote:
> /me poking around here because ...

thx for noting.
 
> Cluster fuzz produced a complaint about toDataURL after this patch: did you
guys
> get bug mail at all?

I guess http://crbug.com/445848 
if there is another issue, plz let me know. anyway, it's not easy for me to
imagine how this commit causes fuzz test failure.

> Drive-by comment: If a user has never called getContext("2d") or
> getContext("3d") on their canvas (m_context has not been created), why should
> toDataURL even attempt to return an image, which the code seems to be doing
> here? What does Mozilla do? /me curious.

all browsers return transparent image.
we discussed about it at msg #2 and #3

On 2014/11/24 17:20:57, junov wrote:
> The current behavior complies with the specification.  The spec says: If the
> canvas has no pixels (i.e. either its horizontal dimension or its vertical
> dimension is zero) then the method must return the string "data:,". (This is
the
> shortest data: URL; it represents the empty string in a text/plain resource.) 

> 
> Also, my interpretation is that if the canvas has a non-empty size but no
> context has been created, to DataURL should encode an image that contains all
> transparent black pixels.

I agree on you. We should preserve the existing behavior.
For example, MDN HTMLCanvasElement.toDataURL show the following example,
https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement.toDataURL

<canvas id="canvas" width="5" height="5"></canvas>
var canvas = document.getElementById("canvas");
var dataURL = canvas.toDataURL();
console.log(dataURL);
// "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNby
// blAAAADElEQVQImWNgoBMAAABpAAFEI8ARAAAAAElFTkSuQmCC"

[Noel Gordon, 2015-01-07 01:24:24]

On 2015/01/06 11:55:56, dshwang wrote:
> On 2015/01/06 10:43:58, noel gordon wrote:
 
> > Drive-by comment: If a user has never called getContext("2d") or
> > getContext("3d") on their canvas (m_context has not been created), why
should
> > toDataURL even attempt to return an image, which the code seems to be doing
> > here? What does Mozilla do? /me curious.
> 
> all browsers return transparent image.
> we discussed about it at msg #2 and #3

OK good, we have compat.

[Noel Gordon, 2015-01-07 01:39:52]

On 2015/01/06 11:55:56, dshwang wrote:

> On 2015/01/06 10:43:58, noel gordon wrote:
> > Cluster fuzz produced a complaint about toDataURL after this patch: did
> > you guys get bug mail at all?
> 
> I guess http://crbug.com/445848 
> if there is another issue, plz let me know. anyway,

https://crbug/445107 (not sure you have access though).

> it's not easy for me to
> imagine how this commit causes fuzz test failure.

Your change was in the list of changes that caused the clusterfuzz report.  I've
examined the reports and instrumented the code, and concluded that the reports
are spurious.  Nothing for you to worry about here.

[Noel Gordon, 2015-01-07 05:33:58]

On 2015/01/07 01:39:52, noel gordon wrote:

> Your change was in the list of changes that caused the clusterfuzz report.

Correction: wasn't this change, sorry.  Clusterfuzz said it was:

 
https://chromium.googlesource.com/chromium/blink.git/+/0654d40e7df1febe133ab6...

We should continue discussion on your bug.