Bug 57409 – [WinCairo] Implement Missing drawWindowsBitmap

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

When playing with the ENABLE(VIDEO) options, I realized there was a missing GraphicsContext method needed for the WinCairo port.

[Brent Fulgham]

Created attachment 87444 [details]
Implementation of missing drawWindowsBitmap for WinCairo port.

[Martin Robinson]

Comment on attachment 87444 [details]
Implementation of missing drawWindowsBitmap for WinCairo port.

View in context: https://bugs.webkit.org/attachment.cgi?id=87444&action=review

It seems like this code and the code in GraphicsContext::releaseWindowsContext are almost identical. Is it possible to refactor it out into a helper function?

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:160
> +                                            CAIRO_FORMAT_ARGB32,
> +                                            image->size().width(),
> +                                            image->size().height(),
> +                                            image->bytesPerRow());
> +

Please line these up with the opening (.

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:166
> +    cairo_scale(m_data->cr, 1.0, -1.0);

This should just be:

cairo_scale(m_data->cr, 1, -1);

[Brent Fulgham]

Created attachment 87617 [details]
Patch

[Brent Fulgham]

Created attachment 87646 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 87646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87646&action=review

> Source/WebCore/platform/graphics/GraphicsContext.h:460
> +            IntSize size() const { return m_bitmapBits.size(); }

Should be a const IntSize&.

> Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp:91
> +GraphicsContext::WindowsBitmap::WindowsBitmap(HDC hdc, GraphicsContext::WindowsBitmap::FromActiveBitmapType)
> +    : m_hdc(hdc)
> +{
> +    m_bitmap = static_cast<HBITMAP>(GetCurrentObject(hdc, OBJ_BITMAP));
> +    m_bitmapBits.initializeFromHBitmap(m_bitmap);
> +}

This will still result in the HDC and HBITMAP being destroyed when the WindowsBitmap is destroyed. That seems undesirable and wrong.

> Source/WebCore/platform/win/WindowsDIB.cpp:31
> +WindowsDIB::WindowsDIB(const HBITMAP& bitmap)

No need for the const&. HBITMAP is just a pointer.

Seems like this class should be in platform/graphics/win.

> Source/WebCore/platform/win/WindowsDIB.cpp:46
> +void WindowsDIB::initializeFromHBitmap(const HBITMAP& bitmap)
> +{
> +    BITMAP bmpInfo;
> +    GetObject(bitmap, sizeof(bmpInfo), &bmpInfo);
> +
> +    m_bitmapBuffer = reinterpret_cast<UInt8*>(bmpInfo.bmBits);
> +    m_bitmapBufferLength = bmpInfo.bmWidthBytes * bmpInfo.bmHeight;
> +    m_size = IntSize(bmpInfo.bmWidth, bmpInfo.bmHeight);
> +    m_bytesPerRow = bmpInfo.bmWidthBytes;
> +    m_bitsPerPixel = bmpInfo.bmBitsPixel;
> +}

Why not just do this in the constructor?

No need for the const&. HBITMAP is just a pointer.

> Source/WebCore/platform/win/WindowsDIB.h:54
> +        IntSize size() const { return m_size; }

Should be a const IntSize&.

[Brent Fulgham]

(In reply to comment #5)
> > Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp:91
> > +GraphicsContext::WindowsBitmap::WindowsBitmap(HDC hdc, GraphicsContext::WindowsBitmap::FromActiveBitmapType)
> > +    : m_hdc(hdc)
> > +{
> > +    m_bitmap = static_cast<HBITMAP>(GetCurrentObject(hdc, OBJ_BITMAP));
> > +    m_bitmapBits.initializeFromHBitmap(m_bitmap);
> > +}
> 
> This will still result in the HDC and HBITMAP being destroyed when the WindowsBitmap is destroyed. That seems undesirable and wrong.

I thought that was the desired function of the existing WindowsBitmap inner class, since that was how it was originally designed.

If the change you suggest is made, we'll have to manually destroy the HDC and HBITMAP context when the WindowsBitmap goes out of scope to retain the same behavior as the current WebKit implementation.  I assumed this object was created to help avoid this manual cleanup.

[Adam Roben (:aroben)]

(In reply to comment #6)
> (In reply to comment #5)
> > > Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp:91
> > > +GraphicsContext::WindowsBitmap::WindowsBitmap(HDC hdc, GraphicsContext::WindowsBitmap::FromActiveBitmapType)
> > > +    : m_hdc(hdc)
> > > +{
> > > +    m_bitmap = static_cast<HBITMAP>(GetCurrentObject(hdc, OBJ_BITMAP));
> > > +    m_bitmapBits.initializeFromHBitmap(m_bitmap);
> > > +}
> > 
> > This will still result in the HDC and HBITMAP being destroyed when the WindowsBitmap is destroyed. That seems undesirable and wrong.
> 
> I thought that was the desired function of the existing WindowsBitmap inner class, since that was how it was originally designed.
> 
> If the change you suggest is made, we'll have to manually destroy the HDC and HBITMAP context when the WindowsBitmap goes out of scope to retain the same behavior as the current WebKit implementation.  I assumed this object was created to help avoid this manual cleanup.

I see. I'd suggest making the current code use OwnPtr to hold the HDC and HBITMAP, and then use your new WindowsDIB class instead of WindowsBitmap. You can leave WindowsBitmap untouched in that case.

[Brent Fulgham]

(In reply to comment #7)
> I see. I'd suggest making the current code use OwnPtr to hold the HDC and HBITMAP, and then use your new WindowsDIB class instead of WindowsBitmap. You can leave WindowsBitmap untouched in that case.

Okay -- I'll give that a shot tomorrow!

Thanks for all the helpful reviews.  :-)

[Brent Fulgham]

(In reply to comment #5)
> > +void WindowsDIB::initializeFromHBitmap(const HBITMAP& bitmap)
> > +{
> > +    BITMAP bmpInfo;
> > +    GetObject(bitmap, sizeof(bmpInfo), &bmpInfo);
> > +
> > +    m_bitmapBuffer = reinterpret_cast<UInt8*>(bmpInfo.bmBits);
> > +    m_bitmapBufferLength = bmpInfo.bmWidthBytes * bmpInfo.bmHeight;
> > +    m_size = IntSize(bmpInfo.bmWidth, bmpInfo.bmHeight);
> > +    m_bytesPerRow = bmpInfo.bmWidthBytes;
> > +    m_bitsPerPixel = bmpInfo.bmBitsPixel;
> > +}
> 
> Why not just do this in the constructor?

I don't have an HBITMAP during construction of the overall WindowsBitmap, so I default construct an empty object, then populate it using the "initializeFromHBitmap" later on.  If that wasn't the case, this could all be moved into the constructor.

[Brent Fulgham]

Created attachment 87767 [details]
Patch

[Brent Fulgham]

Created attachment 87769 [details]
Patch

[Build Bot]

Attachment 87769 [details] did not build on win:
Build output: http://queues.webkit.org/results/8311403

[Brent Fulgham]

Created attachment 87775 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 87775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87775&action=review

It seems buggy that releaseWindowsContext doesn't select the HDC's original bitmap back into it before calling ::DeleteDC. That's not a new problem, though.

This is looking good. Let's do one more pass to nail down the last few things.

> Source/WebCore/platform/graphics/GraphicsContext.h:447
> +

I don't think this extra line is needed.

> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:101
> +    OwnPtr<HBITMAP> bitmap(static_cast<HBITMAP>(GetCurrentObject(hdc, OBJ_BITMAP)));

The more modern way of writing this is to use assignment and adoptPtr.

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:99
> +static void drawBitmapToContext(GraphicsContextPlatformPrivate* context, const WindowsDIB& windowsDIB, double tx, double ty)

tx/ty would be better as an IntSize or FloatSize. (I think only ints are passed in?)

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:137
> +    OwnPtr<HBITMAP> bitmap(static_cast<HBITMAP>(GetCurrentObject(hdc, OBJ_BITMAP)));

Same comment here about adoptPtr.

> Source/WebCore/platform/graphics/win/WindowsDIB.h:39
> +class WindowsDIB {

Since this class doesn't hold onto the DIB itself, maybe a name like DIBData or DIBPixelData would be better.

> Source/WebCore/platform/graphics/win/WindowsDIB.h:48
> +        WindowsDIB(HBITMAP);

This should be marked explicit.

> Source/WebCore/platform/graphics/win/WindowsDIB.h:50
> +        void initializeFromHBitmap(const HBITMAP&);

No need for the const&. I think "FromHBitmap" could be removed, too.

[Brent Fulgham]

(In reply to comment #14)
> (From update of attachment 87775 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87775&action=review
> 
> It seems buggy that releaseWindowsContext doesn't select the HDC's original bitmap back into it before calling ::DeleteDC. That's not a new problem, though.

Wouldn't we have to cache the original bitmap someplace?  Or is there a stock bitmap we can reselect?  Either way, I'd prefer to do that as a separate issue to avoid breaking something unexpected.

> > Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:99
> > +static void drawBitmapToContext(GraphicsContextPlatformPrivate* context, const WindowsDIB& windowsDIB, double tx, double ty)
> 
> tx/ty would be better as an IntSize or FloatSize. (I think only ints are passed in?)

These are x,y coordinates to translate to.  I could use an IntPoint, which might be clearer.

Cairo expects to deal with doubles for these two values, so it seemed easier just to take the arguments as doubles, even though the original values are integers as you point out.

I'll switch to an IntPoint for the argument.

[Brent Fulgham]

Created attachment 87795 [details]
Patch

[Adam Roben (:aroben)]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 87775 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=87775&action=review
> > 
> > It seems buggy that releaseWindowsContext doesn't select the HDC's original bitmap back into it before calling ::DeleteDC. That's not a new problem, though.
> 
> Wouldn't we have to cache the original bitmap someplace?  Or is there a stock bitmap we can reselect?  Either way, I'd prefer to do that as a separate issue to avoid breaking something unexpected.

Yes, we'd have to cache the original bitmap. And yes, this should not be done as part of this patch.

> > > Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:99
> > > +static void drawBitmapToContext(GraphicsContextPlatformPrivate* context, const WindowsDIB& windowsDIB, double tx, double ty)
> > 
> > tx/ty would be better as an IntSize or FloatSize. (I think only ints are passed in?)
> 
> These are x,y coordinates to translate to.  I could use an IntPoint, which might be clearer.

Usually translation is talked about as being a translation by a certain distance, not a translation to a certain point (though the two are of course equivalent). So I'd still argue that IntSize is better than IntPoint.

[Adam Roben (:aroben)]

Comment on attachment 87795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87795&action=review

> Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp:71
> +    UInt8* storage = 0;

You could make this a void* to avoid the cast.

[Brent Fulgham]

(In reply to comment #17)
> > These are x,y coordinates to translate to.  I could use an IntPoint, which might be clearer.
> 
> Usually translation is talked about as being a translation by a certain distance, not a translation to a certain point (though the two are of course equivalent). So I'd still argue that IntSize is better than IntPoint.

Ok.  IntSize it is!

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/82640