Bug 54587 - Tiled backing store area is too big.
Summary: Tiled backing store area is too big.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Viatcheslav Ostapenko
URL:
Keywords: Qt
Depends on:
Blocks: 53894
  Show dependency treegraph
 
Reported: 2011-02-16 14:13 PST by Viatcheslav Ostapenko
Modified: 2011-02-18 17:12 PST (History)
4 users (show)

See Also:


Attachments
Divide inflate delta by 2 to fix area calculation. (2.09 KB, patch)
2011-02-16 14:20 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Fix changelog. (2.06 KB, patch)
2011-02-16 14:27 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Fix also webkit2 tile area calculation. (4.28 KB, patch)
2011-02-16 16:59 PST, Viatcheslav Ostapenko
kenneth: review-
Details | Formatted Diff | Diff
Add comment why delta should be divided by 2 (4.52 KB, patch)
2011-02-17 07:35 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2011-02-16 14:13:09 PST
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)
Comment 1 Viatcheslav Ostapenko 2011-02-16 14:20:29 PST
Created attachment 82694 [details]
Divide inflate delta by 2 to fix area calculation.
Comment 2 Viatcheslav Ostapenko 2011-02-16 14:27:28 PST
Created attachment 82696 [details]
Fix changelog.
Comment 3 Andreas Kling 2011-02-16 15:56:50 PST
Dropping [Qt] prefix as this is not a Qt-specific bug.
Comment 4 Andreas Kling 2011-02-16 15:57:22 PST
Comment on attachment 82696 [details]
Fix changelog.

The WK2 TiledDrawingAreaProxy has the same logic, should the change not be done there, too?
Comment 5 Viatcheslav Ostapenko 2011-02-16 16:59:03 PST
Created attachment 82723 [details]
Fix also webkit2 tile area calculation.
Comment 6 Kenneth Rohde Christiansen 2011-02-17 01:07:38 PST
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))
Comment 7 Viatcheslav Ostapenko 2011-02-17 07:05:09 PST
(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.
Comment 8 Kenneth Rohde Christiansen 2011-02-17 07:10:22 PST
(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
Comment 9 Viatcheslav Ostapenko 2011-02-17 07:12:08 PST
> Better not use inflate then :-) and just use setWidth instead! etc

Ok. As you wish! ;)
Comment 10 Viatcheslav Ostapenko 2011-02-17 07:20:18 PST
> 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.
Comment 11 Kenneth Rohde Christiansen 2011-02-17 07:21:33 PST
Ok, then lets add a comment in the code. Apparently it tricked more than one person.
Comment 12 Kenneth Rohde Christiansen 2011-02-17 07:23:47 PST
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.
Comment 13 Viatcheslav Ostapenko 2011-02-17 07:35:57 PST
Created attachment 82801 [details]
Add comment why delta should be divided by 2
Comment 14 WebKit Commit Bot 2011-02-18 17:12:13 PST
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>
Comment 15 WebKit Commit Bot 2011-02-18 17:12:18 PST
All reviewed patches have been landed.  Closing bug.