[Cairo] ImageBuffer::toDataURL(): improve error handling, add mimeType ASSERTs
Created attachment 136437 [details] Patch
Punt: adding possible cairo reviewers.
Comment on attachment 136437 [details] Patch Does this make the Gtk port pass more tests? Are there results which will need updating?
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.
Comment on attachment 136437 [details] Patch So which (if any) ports use this code?
The Cairo port I believe.
Comment on attachment 136437 [details] Patch There is a WinCairo port, and I think that Gtk may use Cairo.
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+.
Indeed. And yes, Gtk uses Cairo, just not here: http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp#L260
> 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?
(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.
(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 ?
(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 :)
(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!
No problem. Filed bug 83973 about GTK canvas/philip/tests/toDataURL.jpeg.alpha.html
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
Fuzz due to bug 83836 HDI backing store CG canvas.
Created attachment 137203 [details] Patch for landing
Comment on attachment 137203 [details] Patch for landing Clearing flags on attachment: 137203 Committed r114204: <http://trac.webkit.org/changeset/114204>
All reviewed patches have been landed. Closing bug.