Bug 37440 – [WINCE] Port tiled backing store

• Component: Platform
• Status: RESOLVED INVALID

[Kwang Yul Seo]

Port tiled backing store to WinCE with SharedBitmap.

[Kwang Yul Seo]

Created attachment 53156 [details]
Patch

This patch depends on SharedBitmap which is not yet upstreamed. I will submit a patch for SharedBitmap soon.

[Zoltan Herczeg]

(In reply to comment #1)
> Created an attachment (id=53156) [details]
> Patch
> 
> This patch depends on SharedBitmap which is not yet upstreamed. I will submit a
> patch for SharedBitmap soon.

Hm, looking at the Qt implementation, the target of paintCheckerPattern is not necessary checkerSize aligned, and might be scaled as well. Otherwise this patch seems ok.

[Kwang Yul Seo]

I updated the SharedBitmap patch in bug 28272 as this patch depends on SharedBitmap.

[Kwang Yul Seo]

(In reply to comment #2)
> Hm, looking at the Qt implementation, the target of paintCheckerPattern is not
> necessary checkerSize aligned, and might be scaled as well. Otherwise this
> patch seems ok.

You are right. I will fix it.

[Eric Seidel (no email)]

What does Tile do?  What ports use it?  I'm still not sure why recent ports have had to add so many image-related classes.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> What does Tile do?  What ports use it?  I'm still not sure why recent ports have had to add so many image-related classes.

Qt uses it, it is a tiled backing store that we are painting to, similar to what coregraphics does for the iphone port. So in that way it is not really related to images, but to painting.

[Adam Roben (:aroben)]

Comment on attachment 53156 [details]
Patch

> +        * platform/graphics/wince/TileWince.cpp: Added.

This file should be named TileWinCE.cpp (see bug 37287).

> +        (WebCore::checkeredBitmap):
> +        (WebCore::Tile::Tile):
> +        (WebCore::Tile::~Tile):
> +        (WebCore::Tile::isDirty):
> +        (WebCore::Tile::isReadyToPaint):
> +        (WebCore::Tile::invalidate):
> +        (WebCore::enclosingRect):
> +        (WebCore::getUpdateRects):
> +        (WebCore::Tile::updateBackBuffer):
> +        (WebCore::Tile::swapBackBufferToFront):
> +        (WebCore::Tile::paint):
> +        (WebCore::Tile::paintCheckerPattern):

Function-level comments are helpful to anyone trying to understand the patch. That includes reviewers and people in the future who are trying to figure out what the point of this patch was (if, e.g., it caused a regression or contained a bug). It's especially good if the comments explain the "why" of the patch, not just the "how".

> +static PassRefPtr<SharedBitmap> checkeredBitmap()
> +{
> +    static RefPtr<SharedBitmap> bitmap;
> +    if (!bitmap) {
> +        bitmap = SharedBitmap::createInstance(true, checkerSize, checkerSize, false);
> +        unsigned key1, key2;
> +        HDC dc = bitmap->getDC(&key1, &key2);
> +        GraphicsContext context(dc);
> +
> +        Color color1(checkerColor1);
> +        Color color2(checkerColor2);
> +        for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> +            bool alternate = y % checkerSize;
> +            for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> +                context.fillRect(FloatRect(x, y, checkerSize / 2, checkerSize / 2),
> +                    alternate ? color1 : color2, DeviceColorSpace);
> +                alternate = !alternate;
> +            }
> +        }
> +
> +        bitmap->releaseDC(dc, key1, key2);
> +    }
> +    return bitmap.release();
> +}

It looks like this function is trying to create the bitmap just once and then return that single cached instance. But that's not the behavior it will have. RefPtr::release nulls out the RefPtr, so if (!bitmap) will always be true.

I think instead you should change this function to return a bare pointer. It's a also slightly nicer style to have two separate functions: a "create" function that always allocates a new bitmap, and a getter that returns a single cached instance. Like this:

static PassRefPtr<SharedBitmap> createCheckeredBitmap()
{
    RefPtr<SharedBitmap> bitmap = SharedBitmap::createInstance(...);
    // Draw the checker pattern
    return bitmap.release();
}

static SharedBitmap* checkeredBitmap()
{
    static SharedBitmap* bitmap = createCheckeredBitmap().releaseRef();
    return bitmap;
}

> +Tile::Tile(TiledBackingStore* backingStore, const Coordinate& tileCoordinate)
> +    : m_backingStore(backingStore)
> +    , m_coordinate(tileCoordinate)
> +    , m_rect(m_backingStore->tileRectForCoordinate(tileCoordinate))
> +    , m_buffer(0)
> +    , m_backBuffer(0)

No need to initialize m_buffer or m_backBuffer here. RefPtr does this for you automatically.

> +static IntRect enclosingRect(HRGN hrgn)
> +{
> +    RECT r;
> +    GetRgnBox(hrgn, &r);
> +    return r;
> +}
> +

This function is only called once. I'm not sure it's worth having a separate function for this.

> +static void getUpdateRects(HRGN region, Vector<IntRect>& rects)
> +{
> +    const int rectThreshold = 10;
> +    const float wastedSpaceThreshold = 0.75f;
> +
> +    IntRect dirtyRect(enclosingRect(region));
> +
> +    DWORD regionDataSize = GetRegionData(region, sizeof(RGNDATA), 0);
> +    if (!regionDataSize) {
> +        rects.append(dirtyRect);
> +        return;
> +    }
> +
> +    Vector<unsigned char> buffer(regionDataSize);
> +    RGNDATA* regionData = reinterpret_cast<RGNDATA*>(buffer.data());
> +    GetRegionData(region, regionDataSize, regionData);
> +    if (regionData->rdh.nCount > rectThreshold) {
> +        rects.append(dirtyRect);
> +        return;
> +    }
> +
> +    double singlePixels = 0.0;
> +    unsigned i;
> +    RECT* rect;
> +    for (i = 0, rect = reinterpret_cast<RECT*>(regionData->Buffer); i < regionData->rdh.nCount; i++, rect++)
> +        singlePixels += (rect->right - rect->left) * (rect->bottom - rect->top);
> +
> +    double unionPixels = dirtyRect.width() * dirtyRect.height();
> +    double wastedSpace = 1.0 - (singlePixels / unionPixels);
> +    if (wastedSpace <= wastedSpaceThreshold) {
> +        rects.append(dirtyRect);
> +        return;
> +    }
> +
> +    for (i = 0, rect = reinterpret_cast<RECT*>(regionData->Buffer); i < regionData->rdh.nCount; i++, rect++)
> +        rects.append(*rect);
> +}

This looks to be a copy of the function of the same name in WebKit/win/WebView.cpp. Can we share the code instead of duplicating it?

> +void Tile::updateBackBuffer()
> +{
> +    if (m_buffer && !isDirty())
> +        return;
> +
> +    if (!m_backBuffer) {
> +        if (!m_buffer)
> +            m_backBuffer = SharedBitmap::createInstance(
> +                    true, m_backingStore->m_tileSize.width(), m_backingStore->m_tileSize.height(), false);

We normally would just put this all on one line. I think you should do that here.

It would be nice if there were an overload of SharedBitmap::createInstance that took a const IntSize&. It would also be nice if it used enums instead of booleans, as I have no idea what "true" and "false" mean here.

> +        else {
> +            // Currently all buffers are updated synchronously at the same time so there is no real need
> +            // to have separate back and front buffers. Just use the existing buffer.

If there's no need for two buffers, why do we have them?

> +            m_backBuffer = m_buffer;
> +            m_buffer = 0;

You can do this more efficiently like this:

m_backBuffer = m_buffer.release();

> +    HRGN nullRegion = CreateRectRgn(0, 0, 0, 0);
> +    m_dirtyRegion.set(nullRegion);

No need for the local nullRegion variable.

> +    int size = dirtyRects.size();
> +    for (int n = 0; n < size; ++n)  {

It's a little better to use size_t instead of int when iterating over a Vector.

> +void Tile::swapBackBufferToFront()
> +{
> +    if (!m_backBuffer)
> +        return;
> +    m_buffer = m_backBuffer;
> +    m_backBuffer = 0;
> +}

You can do this more efficiently like this:

m_buffer = m_backBuffer.release();

> +void Tile::paint(GraphicsContext* context, const IntRect& rect)
> +{
> +    if (!m_buffer)
> +        return;
> +
> +    IntRect target = intersection(rect, m_rect);
> +    IntRect source((target.x() - m_rect.x()),
> +                   (target.y() - m_rect.y()),

No need for the parentheses around these expressions. I'm also not sure all the newlines are making this more readable.

> +void Tile::paintCheckerPattern(GraphicsContext* context, const FloatRect& target)
> +{
> +    FloatRect tileRectIn(0, 0, checkerSize, checkerSize);
> +    context->drawBitmapPattern(checkeredBitmap().get(),
> +                               tileRectIn,
> +                               AffineTransform(),
> +                               FloatPoint(0, 0),

You can just say FloatPoint()

> +                               CompositeCopy,
> +                               target,
> +                               IntSize(checkerSize, checkerSize));

Is there no way to get the size from checkeredBitmap? It seems unfortunate to have to use the constants again here.

Again, I don't think all the newlines are helping here.

[Young Han Lee]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=53156) [details] [details]
> > Patch
> > 
> > This patch depends on SharedBitmap which is not yet upstreamed. I will submit a
> > patch for SharedBitmap soon.
> 
> Hm, looking at the Qt implementation, the target of paintCheckerPattern is not necessary checkerSize aligned, and might be scaled as well. Otherwise this patch seems ok.

The target doesn't be scaled and the checker is always displayed in same size.

[Young Han Lee]

(In reply to comment #7)
> (From update of attachment 53156 [details])
> > +        * platform/graphics/wince/TileWince.cpp: Added.
> 
> This file should be named TileWinCE.cpp (see bug 37287).
> 
> > +        (WebCore::checkeredBitmap):
> > +        (WebCore::Tile::Tile):
> > +        (WebCore::Tile::~Tile):
> > +        (WebCore::Tile::isDirty):
> > +        (WebCore::Tile::isReadyToPaint):
> > +        (WebCore::Tile::invalidate):
> > +        (WebCore::enclosingRect):
> > +        (WebCore::getUpdateRects):
> > +        (WebCore::Tile::updateBackBuffer):
> > +        (WebCore::Tile::swapBackBufferToFront):
> > +        (WebCore::Tile::paint):
> > +        (WebCore::Tile::paintCheckerPattern):
> 
> Function-level comments are helpful to anyone trying to understand the patch. That includes reviewers and people in the future who are trying to figure out what the point of this patch was (if, e.g., it caused a regression or contained a bug). It's especially good if the comments explain the "why" of the patch, not just the "how".
> 
> > +static PassRefPtr<SharedBitmap> checkeredBitmap()
> > +{
> > +    static RefPtr<SharedBitmap> bitmap;
> > +    if (!bitmap) {
> > +        bitmap = SharedBitmap::createInstance(true, checkerSize, checkerSize, false);
> > +        unsigned key1, key2;
> > +        HDC dc = bitmap->getDC(&key1, &key2);
> > +        GraphicsContext context(dc);
> > +
> > +        Color color1(checkerColor1);
> > +        Color color2(checkerColor2);
> > +        for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> > +            bool alternate = y % checkerSize;
> > +            for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> > +                context.fillRect(FloatRect(x, y, checkerSize / 2, checkerSize / 2),
> > +                    alternate ? color1 : color2, DeviceColorSpace);
> > +                alternate = !alternate;
> > +            }
> > +        }
> > +
> > +        bitmap->releaseDC(dc, key1, key2);
> > +    }
> > +    return bitmap.release();
> > +}
> 
> It looks like this function is trying to create the bitmap just once and then return that single cached instance. But that's not the behavior it will have. RefPtr::release nulls out the RefPtr, so if (!bitmap) will always be true.
> 
> I think instead you should change this function to return a bare pointer. It's a also slightly nicer style to have two separate functions: a "create" function that always allocates a new bitmap, and a getter that returns a single cached instance. Like this:
> 
> static PassRefPtr<SharedBitmap> createCheckeredBitmap()
> {
>     RefPtr<SharedBitmap> bitmap = SharedBitmap::createInstance(...);
>     // Draw the checker pattern
>     return bitmap.release();
> }
> 
> static SharedBitmap* checkeredBitmap()
> {
>     static SharedBitmap* bitmap = createCheckeredBitmap().releaseRef();
>     return bitmap;
> }
> 
> > +Tile::Tile(TiledBackingStore* backingStore, const Coordinate& tileCoordinate)
> > +    : m_backingStore(backingStore)
> > +    , m_coordinate(tileCoordinate)
> > +    , m_rect(m_backingStore->tileRectForCoordinate(tileCoordinate))
> > +    , m_buffer(0)
> > +    , m_backBuffer(0)
> 
> No need to initialize m_buffer or m_backBuffer here. RefPtr does this for you automatically.
> 
> > +static IntRect enclosingRect(HRGN hrgn)
> > +{
> > +    RECT r;
> > +    GetRgnBox(hrgn, &r);
> > +    return r;
> > +}
> > +
> 
> This function is only called once. I'm not sure it's worth having a separate function for this.
> 
> > +static void getUpdateRects(HRGN region, Vector<IntRect>& rects)
> > +{
> > +    const int rectThreshold = 10;
> > +    const float wastedSpaceThreshold = 0.75f;
> > +
> > +    IntRect dirtyRect(enclosingRect(region));
> > +
> > +    DWORD regionDataSize = GetRegionData(region, sizeof(RGNDATA), 0);
> > +    if (!regionDataSize) {
> > +        rects.append(dirtyRect);
> > +        return;
> > +    }
> > +
> > +    Vector<unsigned char> buffer(regionDataSize);
> > +    RGNDATA* regionData = reinterpret_cast<RGNDATA*>(buffer.data());
> > +    GetRegionData(region, regionDataSize, regionData);
> > +    if (regionData->rdh.nCount > rectThreshold) {
> > +        rects.append(dirtyRect);
> > +        return;
> > +    }
> > +
> > +    double singlePixels = 0.0;
> > +    unsigned i;
> > +    RECT* rect;
> > +    for (i = 0, rect = reinterpret_cast<RECT*>(regionData->Buffer); i < regionData->rdh.nCount; i++, rect++)
> > +        singlePixels += (rect->right - rect->left) * (rect->bottom - rect->top);
> > +
> > +    double unionPixels = dirtyRect.width() * dirtyRect.height();
> > +    double wastedSpace = 1.0 - (singlePixels / unionPixels);
> > +    if (wastedSpace <= wastedSpaceThreshold) {
> > +        rects.append(dirtyRect);
> > +        return;
> > +    }
> > +
> > +    for (i = 0, rect = reinterpret_cast<RECT*>(regionData->Buffer); i < regionData->rdh.nCount; i++, rect++)
> > +        rects.append(*rect);
> > +}
> 
> This looks to be a copy of the function of the same name in WebKit/win/WebView.cpp. Can we share the code instead of duplicating it?
> 
> > +void Tile::updateBackBuffer()
> > +{
> > +    if (m_buffer && !isDirty())
> > +        return;
> > +
> > +    if (!m_backBuffer) {
> > +        if (!m_buffer)
> > +            m_backBuffer = SharedBitmap::createInstance(
> > +                    true, m_backingStore->m_tileSize.width(), m_backingStore->m_tileSize.height(), false);
> 
> We normally would just put this all on one line. I think you should do that here.
> 
> It would be nice if there were an overload of SharedBitmap::createInstance that took a const IntSize&. It would also be nice if it used enums instead of booleans, as I have no idea what "true" and "false" mean here.
> 
> > +        else {
> > +            // Currently all buffers are updated synchronously at the same time so there is no real need
> > +            // to have separate back and front buffers. Just use the existing buffer.
> 
> If there's no need for two buffers, why do we have them?
> 
> > +            m_backBuffer = m_buffer;
> > +            m_buffer = 0;
> 
> You can do this more efficiently like this:
> 
> m_backBuffer = m_buffer.release();
> 
> > +    HRGN nullRegion = CreateRectRgn(0, 0, 0, 0);
> > +    m_dirtyRegion.set(nullRegion);
> 
> No need for the local nullRegion variable.
> 
> > +    int size = dirtyRects.size();
> > +    for (int n = 0; n < size; ++n)  {
> 
> It's a little better to use size_t instead of int when iterating over a Vector.
> 
> > +void Tile::swapBackBufferToFront()
> > +{
> > +    if (!m_backBuffer)
> > +        return;
> > +    m_buffer = m_backBuffer;
> > +    m_backBuffer = 0;
> > +}
> 
> You can do this more efficiently like this:
> 
> m_buffer = m_backBuffer.release();
> 
> > +void Tile::paint(GraphicsContext* context, const IntRect& rect)
> > +{
> > +    if (!m_buffer)
> > +        return;
> > +
> > +    IntRect target = intersection(rect, m_rect);
> > +    IntRect source((target.x() - m_rect.x()),
> > +                   (target.y() - m_rect.y()),
> 
> No need for the parentheses around these expressions. I'm also not sure all the newlines are making this more readable.
> 
> > +void Tile::paintCheckerPattern(GraphicsContext* context, const FloatRect& target)
> > +{
> > +    FloatRect tileRectIn(0, 0, checkerSize, checkerSize);
> > +    context->drawBitmapPattern(checkeredBitmap().get(),
> > +                               tileRectIn,
> > +                               AffineTransform(),
> > +                               FloatPoint(0, 0),
> 
> You can just say FloatPoint()
> 
> > +                               CompositeCopy,
> > +                               target,
> > +                               IntSize(checkerSize, checkerSize));
> 
> Is there no way to get the size from checkeredBitmap? It seems unfortunate to have to use the constants again here.
> 
> Again, I don't think all the newlines are helping here.

I accepted all your points except a few works seems like overwork. We could consider it later.

[Young Han Lee]

Created attachment 56574 [details]
patch

[Young Han Lee]

Comment on attachment 56574 [details]
patch

There were some mistakes. sorry.

[Young Han Lee]

Created attachment 56686 [details]
new patch

Here is the patch I intended to attach, sorry for the confusion.

[Simon Hausmann]

Comment on attachment 56686 [details]
new patch

WebCore/platform/graphics/wince/TileWinCE.cpp:117
 +  static void getUpdateRects(HRGN region, Vector<IntRect>& rects)
Before this function you define the convenient enclosingRect() helper function, and a few lines into this function you call GetRgnBox instead of the newly defined helper functions. I suggest to make use of it :)

The rest of the patch looks okay to me, although I pity you guys for having to use HRGN :)

[Kenneth Rohde Christiansen]

Comment on attachment 56686 [details]
new patch

r- due to Simon Hausmann's comment.

[Ahmad Saleem]

@ap - WINCE port exist? I don't think so.. Can we close this?