Bug 54587 – Tiled backing store area is too big.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Viatcheslav Ostapenko]

Tiled backing store area is too big.

It calculated like this:
keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f));
keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f));

and original intention was to get rect of size keepAreaMultiplier * visibleRect, but inflateX and inflateY methods defined like this:

void inflateX(float dx) {
    m_location.setX(m_location.x() - dx);
    m_size.setWidth(m_size.width() + dx + dx);
}
void inflateY(float dy) {
    m_location.setY(m_location.y() - dy);
    m_size.setHeight(m_size.height() + dy + dy);
}

so, they add delta to width/height 2 times, so in real area is calculated like 

width + width * (ratio - 1) * 2 = width * (2 * ratio - 1)

[Viatcheslav Ostapenko]

Created attachment 82694 [details]
Divide inflate delta by 2 to fix area calculation.

[Viatcheslav Ostapenko]

Created attachment 82696 [details]
Fix changelog.

[Andreas Kling]

Dropping [Qt] prefix as this is not a Qt-specific bug.

[Andreas Kling]

Comment on attachment 82696 [details]
Fix changelog.

The WK2 TiledDrawingAreaProxy has the same logic, should the change not be done there, too?

[Viatcheslav Ostapenko]

Created attachment 82723 [details]
Fix also webkit2 tile area calculation.

[Kenneth Rohde Christiansen]

Comment on attachment 82723 [details]
Fix also webkit2 tile area calculation.

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

> Source/WebCore/ChangeLog:8
> +        Tiled backing store area is too big.
> +        Error in area calculcation causes size of backing store
> +        up to 6 times bigger than viewport with default multipliers.
> +        https://bugs.webkit.org/show_bug.cgi?id=54587

Why dont you explain how it is calculating wrong and why it wouldnt be better to change the default multipliers?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:217
> -    keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f));
> -    keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f));
> +    keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f) / 2);
> +    keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f) / 2);

I dont understand why this is right. It is called a multiplier. So if we have size 10, and a multiplier of two, we would expect a result of 20. ie 10 + 10 * 1 (10.inflate(10 * (2 - 1))

[Viatcheslav Ostapenko]

(In reply to comment #6)
> (From update of attachment 82723 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82723&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Tiled backing store area is too big.
> > +        Error in area calculcation causes size of backing store
> > +        up to 6 times bigger than viewport with default multipliers.
> > +        https://bugs.webkit.org/show_bug.cgi?id=54587
> 
> Why dont you explain how it is calculating wrong and why it wouldnt be better to change the default multipliers?
> 
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:217
> > -    keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f));
> > -    keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f));
> > +    keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f) / 2);
> > +    keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f) / 2);
> 
> I dont understand why this is right. It is called a multiplier. So if we have size 10, and a multiplier of two, we would expect a result of 20. ie 10 + 10 * 1 (10.inflate(10 * (2 - 1))

Kenneth, have you looked at Inflate function? I've posted it into explanation. ;)
It adds delta 2 times (m_size.width() + dx + dx).
So, if you have width 20 and multiplier 2, than you will pass delta to inflate as 10 * (2 - 1) = 10. Inside inflate will do width + dx + dx , so instead of 20 you are getting 30.
If you have multiplier 4.5 as in wk2, you will pass delta as 10 * (4.5 - 1) = 35 and after inflate you will get 10 + 35 + 35 = 80 instead of 45.
Make it easy - put a breakpoint, calculate what do you expect and check what you get.

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 82723 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=82723&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        Tiled backing store area is too big.
> > > +        Error in area calculcation causes size of backing store
> > > +        up to 6 times bigger than viewport with default multipliers.
> > > +        https://bugs.webkit.org/show_bug.cgi?id=54587
> > 
> > Why dont you explain how it is calculating wrong and why it wouldnt be better to change the default multipliers?
> > 
> > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:217
> > > -    keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f));
> > > -    keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f));
> > > +    keepRect.inflateX(visibleRect.width() * (m_keepAreaMultiplier.width() - 1.f) / 2);
> > > +    keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f) / 2);
> > 
> > I dont understand why this is right. It is called a multiplier. So if we have size 10, and a multiplier of two, we would expect a result of 20. ie 10 + 10 * 1 (10.inflate(10 * (2 - 1))
> 
> Kenneth, have you looked at Inflate function? I've posted it into explanation. ;)
> It adds delta 2 times (m_size.width() + dx + dx).
> So, if you have width 20 and multiplier 2, than you will pass delta to inflate as 10 * (2 - 1) = 10. Inside inflate will do width + dx + dx , so instead of 20 you are getting 30.
> If you have multiplier 4.5 as in wk2, you will pass delta as 10 * (4.5 - 1) = 35 and after inflate you will get 10 + 35 + 35 = 80 instead of 45.
> Make it easy - put a breakpoint, calculate what do you expect and check what you get.

Better not use inflate then :-) and just use setWidth instead! etc

[Viatcheslav Ostapenko]

> Better not use inflate then :-) and just use setWidth instead! etc

Ok. As you wish! ;)

[Viatcheslav Ostapenko]

> Better not use inflate then :-) and just use setWidth instead! etc

Oh! No! That's not right.
Rect position also needs to be adjusted.
IMHO, so far inflate is the cleanest solution.

[Kenneth Rohde Christiansen]

Ok, then lets add a comment in the code. Apparently it tricked more than one person.

[Kenneth Rohde Christiansen]

Comment on attachment 82723 [details]
Fix also webkit2 tile area calculation.

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

r=me with the comment

>> Source/WebCore/platform/graphics/TiledBackingStore.cpp:217
>> +    keepRect.inflateY(visibleRect.height() * (m_keepAreaMultiplier.height() - 1.f) / 2);
> 
> I dont understand why this is right. It is called a multiplier. So if we have size 10, and a multiplier of two, we would expect a result of 20. ie 10 + 10 * 1 (10.inflate(10 * (2 - 1))

// Inflates to both sides, so divide by 2.

[Viatcheslav Ostapenko]

Created attachment 82801 [details]
Add comment why delta should be divided by 2

[WebKit Commit Bot]

Comment on attachment 82801 [details]
Add comment why delta should be divided by 2

Clearing flags on attachment: 82801

Committed r79054: <http://trac.webkit.org/changeset/79054>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.