Bug 51297 – Clarify ImageBuffer and ImageData relationship

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Simon Fraser (smfr)]

Right now we have platform/graphics/ImageBuffer, and html/ImageData.

ImageBuffer uses ImageData, which is clearly a layering violation.

ImageBuffer doesn't allow direct pixel access, but ImageBuffer does. However, you can't get a GraphicsContext from an ImageData.

Do we need both these classes? Would it be possible to merge them into something that both allows direct pixel access, and is able to provide a GraphicsContext?

[Dirk Schulze]

(In reply to comment #0)
> Right now we have platform/graphics/ImageBuffer, and html/ImageData.
> 
> ImageBuffer uses ImageData, which is clearly a layering violation.
This is correct and we also had some discussions about this in the past, but no one investigated on this yet.

> 
> ImageBuffer doesn't allow direct pixel access, but ImageBuffer does. However, you can't get a GraphicsContext from an ImageData.
Hm, I guess you mean that ImageBuffer it self doesn't provide a way for pixel manipulation but ImageData does in your first sentence. Nevertheless, ImageData in ImageBuffer is not the best solution. Talked with Niko about this topic and IIRC, his solutions was to return a byte array instead of ImageData on ImageBuffer and ImageData relies to that byte array in case of HTML Canvas.

To your last point. It is necessary for Filters that ImageBuffer provides a way for direct pixel manipulation but stillreturns an Image afterwards. E.g. feColorMatrix needs direct pixel manipulation, but if you want to composite the result of feColorMatrix you need the Image. But IIRC SVG filters never ask for a GraphicsContext after any direct pixel manipulation.

As conclusion, we shouldn't _need_ ImageData in ImageBuffer. ImageData is just used by HTML Canvas and should just ask for a byte array and the width and height of the requested image area. SVG on the other side doesn't even need ImageData. It was just the only way to get a pixel/byte array right now.

Just curious. Why did you assigned the bug to me? :-)

[Simon Fraser (smfr)]

(In reply to comment #1)
> (In reply to comment #0)
> > Right now we have platform/graphics/ImageBuffer, and html/ImageData.
> > 
> > ImageBuffer uses ImageData, which is clearly a layering violation.
> This is correct and we also had some discussions about this in the past, but no one investigated on this yet.
> 
> > 
> > ImageBuffer doesn't allow direct pixel access, but ImageBuffer does. However, you can't get a GraphicsContext from an ImageData.
> Hm, I guess you mean that ImageBuffer it self doesn't provide a way for pixel manipulation but ImageData does in your first sentence.

Right, sorry.

> Nevertheless, ImageData in ImageBuffer is not the best solution. Talked with Niko about this topic and IIRC, his solutions was to return a byte array instead of ImageData on ImageBuffer and ImageData relies to that byte array in case of HTML Canvas.

ImageData is really just a wrapper around a byte array. The real question is whether this always copies (premultiplied or not), or whether you allow direct pixel access.

I wonder if the lack of direct pixel access on ImageBuffer is because some platforms might not be able to implement this?

> To your last point. It is necessary for Filters that ImageBuffer provides a way for direct pixel manipulation but stillreturns an Image afterwards. E.g. feColorMatrix needs direct pixel manipulation, but if you want to composite the result of feColorMatrix you need the Image. But IIRC SVG filters never ask for a GraphicsContext after any direct pixel manipulation.

I'm looking at this outside of Filters (I want to use something like ContextShadow for CSS shadows, and ImageBuffer is a perfect fit, other than the lack of direct pixel access).

> As conclusion, we shouldn't _need_ ImageData in ImageBuffer. ImageData is just used by HTML Canvas and should just ask for a byte array and the width and height of the requested image area. SVG on the other side doesn't even need ImageData. It was just the only way to get a pixel/byte array right now.

Another thing I'd like to consider is why it's not easier to create an Image that simply wraps an ImageBuffer. GraphicsContext has a lot of method duplication for drawImageBuffer and drawImage, and I don't really see why we need both.

> Just curious. Why did you assigned the bug to me? :-)

Feel free to assign it to Nobody. I just though you might be able to answer the question :)

[Dirk Schulze]

(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > Right now we have platform/graphics/ImageBuffer, and html/ImageData.
> > > 
> > > ImageBuffer uses ImageData, which is clearly a layering violation.
> > This is correct and we also had some discussions about this in the past, but no one investigated on this yet.
> > 
> > > 
> > > ImageBuffer doesn't allow direct pixel access, but ImageBuffer does. However, you can't get a GraphicsContext from an ImageData.
> > Hm, I guess you mean that ImageBuffer it self doesn't provide a way for pixel manipulation but ImageData does in your first sentence.
> 
> Right, sorry.
> 
> > Nevertheless, ImageData in ImageBuffer is not the best solution. Talked with Niko about this topic and IIRC, his solutions was to return a byte array instead of ImageData on ImageBuffer and ImageData relies to that byte array in case of HTML Canvas.
> 
> ImageData is really just a wrapper around a byte array. The real question is whether this always copies (premultiplied or not), or whether you allow direct pixel access.
Right, but IIRC ImageData is a wrapper for CanvasPixelArray which is a wrapper for byte array :-P

> 
> I wonder if the lack of direct pixel access on ImageBuffer is because some platforms might not be able to implement this?
The most platforms support it (otherwise they won't be able to use HTML Canvas ImageData anyway).

> 
> > To your last point. It is necessary for Filters that ImageBuffer provides a way for direct pixel manipulation but stillreturns an Image afterwards. E.g. feColorMatrix needs direct pixel manipulation, but if you want to composite the result of feColorMatrix you need the Image. But IIRC SVG filters never ask for a GraphicsContext after any direct pixel manipulation.
> 
> I'm looking at this outside of Filters (I want to use something like ContextShadow for CSS shadows, and ImageBuffer is a perfect fit, other than the lack of direct pixel access).
Sure.

> 
> > As conclusion, we shouldn't _need_ ImageData in ImageBuffer. ImageData is just used by HTML Canvas and should just ask for a byte array and the width and height of the requested image area. SVG on the other side doesn't even need ImageData. It was just the only way to get a pixel/byte array right now.
> 
> Another thing I'd like to consider is why it's not easier to create an Image that simply wraps an ImageBuffer. GraphicsContext has a lot of method duplication for drawImageBuffer and drawImage, and I don't really see why we need both.
Can you explain it a bit more please?

[Simon Fraser (smfr)]

> > Another thing I'd like to consider is why it's not easier to create an Image that simply wraps an ImageBuffer. GraphicsContext has a lot of method duplication for drawImageBuffer and drawImage, and I don't really see why we need both.
> Can you explain it a bit more please?

Rather than have GraphicsContext::drawImage() and GraphicsContext::drawImageBuffer() methods, why not make it really cheap to say

ImageBuffer* buffer;
buffer = .......

I see no reason why a bitmap image can't be created by simply wrapping an ImageBuffer.

RefPtr<Image> image = BitmapImage::create(buffer);

and then eliminate all the GraphicsContext::drawImageBuffer() methods.

[Dirk Schulze]

(In reply to comment #4)
> > > Another thing I'd like to consider is why it's not easier to create an Image that simply wraps an ImageBuffer. GraphicsContext has a lot of method duplication for drawImageBuffer and drawImage, and I don't really see why we need both.
> > Can you explain it a bit more please?
> 
> Rather than have GraphicsContext::drawImage() and GraphicsContext::drawImageBuffer() methods, why not make it really cheap to say
> 
> ImageBuffer* buffer;
> buffer = .......
> 
> I see no reason why a bitmap image can't be created by simply wrapping an ImageBuffer.
> 
> RefPtr<Image> image = BitmapImage::create(buffer);
> 
> and then eliminate all the GraphicsContext::drawImageBuffer() methods.

This is from a mail of dhyatt on webkit-dev - the reason why we have drawImage and drawImageBuffer. (and IIRC this was also a compromise to match the behavior of all platforms, I need to read the whole discussion again, to be sure).

"The changes to ImageBuffer have landed.  Here is what port authors need to know:

Image* image() on ImageBuffer is gone.

It has been replaced with:

PassRefPtr<Image> copyImage()

This function should always simply copy the image.   It is used in any place where you want to get a snapshot of the ImageBuffer and not be broken if the ImageBuffer subsequently changes.

For drawing, callers should now use drawImageBuffer on GraphicsContext instead of drawImage.  drawImageBuffer internally just turns around and calls a draw function on ImageBuffer.  clipToImageBuffer now also turns around and calls clip on the ImageBuffer."

I also checked PassRefPtr<ImageData> getPre(Un)multipliedImageData(...). The only place where we really need ImageData is:

html/canvas/CanvasRenderingContext2D.cpp:    return buffer->getUnmultipliedImageData(scaledRect);

All other callers just need the pixel data itself. Shouldn't be a problem to change this.

[Dirk Schulze]

Regarding to ImageData <-> ImageBuffer.
Like we discussed, ImageBuffer shouldn't use ImageData. ImageData doesn't need a relation to GraphicsContext, since it is just used as a pixel array with information of width and height of the canvas and is used by HTML Canvas for get/putImageData.
I guess I misunderstand your last comment. You suggest that ImageData directly manipulate the ImageBuffer, so that we don't need to call putImageData afterwards? I don't think so, because changes on the pixel array doesn't affect any ImageBuffer at all until we write the data back. So we definitely need a copy of the pixel information. The same for most of the SVG filters.
I suggest to return a ref counted ByteArray instead of ImageData in ImageBuffer. This way we get a copy and have no violation of the layering. What do you think Simon?

Btw. Shouldn't we move ImageData.h/cpp/idl from WebCore/html to WebCore/html/canvas?

[Dirk Schulze]

Created attachment 77749 [details]
Patch

[WebKit Review Bot]

Attachment 77749 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7238300

[WebKit Review Bot]

Attachment 77749 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7264321

[Early Warning System Bot]

Attachment 77749 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7301309

[Dirk Schulze]

Created attachment 77752 [details]
Patch

[WebKit Review Bot]

Attachment 77752 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7245301

[Dirk Schulze]

Created attachment 77754 [details]
Patch

[WebKit Review Bot]

Attachment 77754 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7200322

[Dirk Schulze]

Created attachment 77755 [details]
Patch

[WebKit Review Bot]

Attachment 77752 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7247333

[Dirk Schulze]

Comment on attachment 77755 [details]
Patch

Clearing flags on attachment: 77755

Committed r74868: <http://trac.webkit.org/changeset/74868>

[Dirk Schulze]

All reviewed patches have been landed.  Closing bug.