The goal is to have one class for the ImageFrame. Currently we have two very similar implementations: one is defined for non CG ports "ImageFrame", which is defined in /platform/image-decoders/ImageDecoder.h, and the second is "FrameData", which is defined in platform/graphics/BitmapImage.h. We begin by moving the pixel data manipulation part in ImageFrame into a separate class and then make it optional (allocated on demand). So for the CG case, BitmapPixels object will not be allocated because all the decoding is done by CG.
Created attachment 283425 [details] Patch
Created attachment 283427 [details] Patch
Created attachment 283429 [details] Patch
Created attachment 283431 [details] Patch
Created attachment 283433 [details] Patch
Created attachment 283437 [details] Patch
Created attachment 283439 [details] Patch
Created attachment 283440 [details] Patch
Created attachment 283443 [details] Patch
Created attachment 283449 [details] Patch
Created attachment 283450 [details] Patch
Created attachment 283461 [details] Patch
Created attachment 283466 [details] Patch
Created attachment 283467 [details] Patch
Created attachment 283469 [details] Patch for review
Created attachment 288426 [details] Patch
Created attachment 288428 [details] Patch
Created attachment 288429 [details] Patch
Created attachment 288447 [details] Patch
Created attachment 288451 [details] Patch
Created attachment 288459 [details] Patch
Created attachment 288461 [details] Patch
Created attachment 288463 [details] Patch
Created attachment 288466 [details] Patch
Created attachment 288468 [details] Patch
Created attachment 288472 [details] Patch
Created attachment 288475 [details] Patch
Comment on attachment 288475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288475&action=review I ran the tests for GTK+ and got mostly the same results, so I don't think this is introducing any regression. > Source/WebCore/platform/graphics/ImageBackingStore.h:38 > +class ImageBackingStore { > +public: This could probably be fast allocated. > Source/WebCore/platform/graphics/ImageBackingStore.h:47 > + static std::unique_ptr<ImageBackingStore> create(const IntSize& size, bool premultiplyAlpha) > + { > + return std::make_unique<ImageBackingStore>(size, premultiplyAlpha); > + } > + > + static std::unique_ptr<ImageBackingStore> create(const ImageBackingStore& other) > + { > + return std::make_unique<ImageBackingStore>(other); > + } Do we need create methods if they just call macke_unique and constructors are public? > Source/WebCore/platform/graphics/ImageBackingStore.h:49 > + ImageBackingStore(const IntSize& size, bool premultiplyAlpha = true) So, premultiplyAlpha is optional here but not in the create method. > Source/WebCore/platform/graphics/ImageBackingStore.h:59 > + if (this == &other) > + return; I don't understand this. > Source/WebCore/platform/graphics/ImageBackingStore.h:71 > + { > + if (!m_pixels.tryReserveCapacity(size.area())) Is it ok to set an empty size? otherwise I would add an assert here. > Source/WebCore/platform/graphics/ImageBackingStore.h:89 > + IntSize size() const { return m_size; } Could we return const IntSize& here? > Source/WebCore/platform/graphics/ImageBackingStore.h:90 > + IntRect frameRect() const { return m_frameRect; } And const IntRect& here? > Source/WebCore/platform/image-decoders/ImageDecoder.h:93 > + IntSize size() const { return m_backingStore->size(); } const IntSize& ? > Source/WebCore/platform/image-decoders/ImageDecoder.h:110 > + void setBackingStore(const ImageBackingStore*); Where is this implemented? > Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:38 > +NativeImagePtr ImageBackingStore::image() const > +{ > + return adoptRef(cairo_image_surface_create_for_data( > + reinterpret_cast<unsigned char*>(const_cast<RGBA32*>(m_pixelsPtr)), > + CAIRO_FORMAT_ARGB32, size().width(), size().height(), size().width() * sizeof(RGBA32))); > +} So, I guess we can remove ImageDecoderCairo.cpp now.
Created attachment 288598 [details] Patch
Comment on attachment 288475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288475&action=review Thanks for the code review and for verifying the layout tests. >> Source/WebCore/platform/graphics/ImageBackingStore.h:38 >> +public: > > This could probably be fast allocated. Done. >> Source/WebCore/platform/graphics/ImageBackingStore.h:47 >> + } > > Do we need create methods if they just call macke_unique and constructors are public? Done. Constructors became private and I have to abandon calling std::make_unique() since it requires the constructor to be public. >> Source/WebCore/platform/graphics/ImageBackingStore.h:49 >> + ImageBackingStore(const IntSize& size, bool premultiplyAlpha = true) > > So, premultiplyAlpha is optional here but not in the create method. Done. The premultiplyAlpha is now optional in both the constructor and the create method. >> Source/WebCore/platform/graphics/ImageBackingStore.h:59 >> + return; > > I don't understand this. You are right. This constructor was originally implemented as an assignment operator. I forgot to change its body to be suitable for a copy constructor. >> Source/WebCore/platform/graphics/ImageBackingStore.h:71 >> + if (!m_pixels.tryReserveCapacity(size.area())) > > Is it ok to set an empty size? otherwise I would add an assert here. A check for !size.isEmpty() its added to this if-statment. >> Source/WebCore/platform/image-decoders/ImageDecoder.h:93 >> + IntSize size() const { return m_backingStore->size(); } > > const IntSize& ? Done. >> Source/WebCore/platform/image-decoders/ImageDecoder.h:110 >> + void setBackingStore(const ImageBackingStore*); > > Where is this implemented? setBackingStore() is not implemented and this declaration is not needed. >> Source/WebCore/platform/image-decoders/cairo/ImageBackingStoreCairo.cpp:38 >> +} > > So, I guess we can remove ImageDecoderCairo.cpp now. You are right. ImageDecoderCairo.cpp is now deleted.
Created attachment 288604 [details] Patch
Comment on attachment 288604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288604&action=review > Source/WebCore/platform/graphics/ImageBackingStore.h:37 > +class ImageBackingStore { I'm surprised that this class doesn't pad rowBytes to some multiple of 4 or 8 bytes. > Source/WebCore/platform/graphics/ImageBackingStore.h:85 > + size_t rectWidthInBytes = rect.width() * sizeof(RGBA32); This is normally called rowBytes > Source/WebCore/platform/graphics/ImageBackingStore.h:98 > + size_t rectWidthInBytes = rect.width() * sizeof(RGBA32); Ditto. > Source/WebCore/platform/graphics/ImageBackingStore.h:107 > + RGBA32* at(int x, int y) const I don't like this name. > Source/WebCore/platform/graphics/ImageBackingStore.h:112 > + void setRGBA(int x, int y, unsigned r, unsigned g, unsigned b, unsigned a) I would call this setPixel() > Source/WebCore/platform/graphics/ImageBackingStore.h:127 > + void overRGBA(RGBA32* dest, unsigned r, unsigned g, unsigned b, unsigned a) I don't like this name.
Created attachment 288626 [details] Patch
Comment on attachment 288626 [details] Patch Clearing flags on attachment: 288626 Committed r205841: <http://trac.webkit.org/changeset/205841>
All reviewed patches have been landed. Closing bug.