Bug 83569 – [Cairo] ImageBuffer::toDataURL(): improve error handling, add mimeType ASSERTs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[noel gordon]

[Cairo] ImageBuffer::toDataURL(): improve error handling, add mimeType ASSERTs

[noel gordon]

Created attachment 136437 [details]
Patch

[noel gordon]

Punt: adding possible cairo reviewers.

[Eric Seidel (no email)]

Comment on attachment 136437 [details]
Patch

Does this make the Gtk port pass more tests?  Are there results which will need updating?

[noel gordon]

No and No.  GTK does not use this function, they have their own. This change is to update the Cairo port implementation to match the other webkit ports, without changing paint/test behavior.

[Eric Seidel (no email)]

Comment on attachment 136437 [details]
Patch

So which (if any) ports use this code?

[noel gordon]

The Cairo port I believe.

[Eric Seidel (no email)]

Comment on attachment 136437 [details]
Patch

There is a WinCairo port, and I think that Gtk may use Cairo.

[Eric Seidel (no email)]

Comment on attachment 136437 [details]
Patch

But it looks reasonable to me.  You should probably give the possible users of this code 24 hours to comment before setting cq+.

[noel gordon]

Indeed.  And yes, Gtk uses Cairo, just not here:
  http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp#L260

[noel gordon]

> You should probably give the possible users of this code 24 hours to comment before setting cq+.

Agree.  They could maybe answer this question: is cairo_surface_write_to_png_stream() thread-safe?

[Martin Robinson]

(In reply to comment #10)
> > You should probably give the possible users of this code 24 hours to comment before setting cq+.
> 
> Agree.  They could maybe answer this question: is cairo_surface_write_to_png_stream() thread-safe?

The GTK+port is a Cairo port, but we have a GTK+-only version of this functionality, because the GTK+ libraries support more types of images than Cairo. Cairo only supports PNG.

I doubt that cairo_surface_write_to_png_stream is thread-safe, so you might consider making a deep copy of the image surface before spinning off the other thread. Is this code threaded now? You'll likely need to do the same thing for the GTK+ port.

[noel gordon]

(In reply to comment #11)
> 
> The GTK+port is a Cairo port, but we have a GTK+-only version of this functionality, because the GTK+ libraries support more types of images than Cairo. Cairo only supports PNG.

Yes that matches my understanding, the GTK+-only version was added in https://bugs.webkit.org/attachment.cgi?id=62763&action=prettypatch. Hmmm, does GTK+ pass or skip canvas/philip/tests/toDataURL.jpeg.alpha.html ?

[noel gordon]

(In reply to comment #11)

> I doubt that cairo_surface_write_to_png_stream is thread-safe, so you might consider making a deep copy of the image surface before spinning off the other thread. Is this code threaded now?
> You'll likely need to do the same thing for the GTK+ port.

A cool Martin thanks. The code is not threaded now. Thread-safety only comes into play with canvas -> blob, which will depend on ImageBuffer since that's where the encoders exist on all ports currently. Seems some ports (Qt, Cairo, and GTK+ if I've understood what you've said) won't support encoding via ImageBuffer in a thread. I need to design for that and it's one reason why I'm assessing the ImageBuffer impl's of each port at this time. Anywhere I see a doubt, I'm marking that ImageBuffer as non-thread-safe. The deep copy idea is good, but I intend to leave that part of canvas->blob to the port maintainers. Would that work for you and GTK+ port?  I'd hate to break you :)

[Martin Robinson]

(In reply to comment #13)
> (In reply to comment #11)
> 
> > I doubt that cairo_surface_write_to_png_stream is thread-safe, so you might consider making a deep copy of the image surface before spinning off the other thread. Is this code threaded now?
> > You'll likely need to do the same thing for the GTK+ port.
> 
> A cool Martin thanks. The code is not threaded now. Thread-safety only comes into play with canvas -> blob, which will depend on ImageBuffer since that's where the encoders exist on all ports currently. Seems some ports (Qt, Cairo, and GTK+ if I've understood what you've said) won't support encoding via ImageBuffer in a thread. I need to design for that and it's one reason why I'm assessing the ImageBuffer impl's of each port at this time. Anywhere I see a doubt, I'm marking that ImageBuffer as non-thread-safe. The deep copy idea is good, but I intend to leave that part of canvas->blob to the port maintainers. Would that work for you and GTK+ port?  I'd hate to break you :)

Sure. That makes sense. Thanks for double-checking all of the ports!

[noel gordon]

No problem. Filed bug 83973 about GTK canvas/philip/tests/toDataURL.jpeg.alpha.html

[WebKit Review Bot]

Comment on attachment 136437 [details]
Patch

Rejecting attachment 136437 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ommit-queue/

Parsed 2 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp
Hunk #1 FAILED at 260.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12407161

[noel gordon]

Fuzz due to bug 83836 HDI backing store CG canvas.

[noel gordon]

Created attachment 137203 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 137203 [details]
Patch for landing

Clearing flags on attachment: 137203

Committed r114204: <http://trac.webkit.org/changeset/114204>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.