Bug 173152 – [Win][HighDPI] Repaint glitches when scrolling.

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

[Per Arne Vollan]

This happens when the device scale factor is non-integral, e.g. 1.25x.

[Per Arne Vollan]

Created attachment 312426 [details]
Patch

[Brent Fulgham]

Comment on attachment 312426 [details]
Patch

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

> Source/WebKit/win/WebView.cpp:954
> +        // FIXME: This is inefficient, we should be able to blit the scroll rectangle in this case as well.

Is this due to a bug in the CA accelerated compositing code path? Or is there a bug in BitBlt in the Windows code?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #2)
> Comment on attachment 312426 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312426&action=review
> 
> > Source/WebKit/win/WebView.cpp:954
> > +        // FIXME: This is inefficient, we should be able to blit the scroll rectangle in this case as well.
> 
> Is this due to a bug in the CA accelerated compositing code path? Or is
> there a bug in BitBlt in the Windows code?

I believe the problem is the conversion from logical scroll delta to scroll delta in pixel coordinates. When the scale factor is non-integral, this conversion is not correct, since we clamp the result. I would prefer a better fix than the current patch :)

[Brent Fulgham]

Comment on attachment 312426 [details]
Patch

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

>>> Source/WebKit/win/WebView.cpp:954
>>> +        // FIXME: This is inefficient, we should be able to blit the scroll rectangle in this case as well.
>> 
>> Is this due to a bug in the CA accelerated compositing code path? Or is there a bug in BitBlt in the Windows code?
> 
> I believe the problem is the conversion from logical scroll delta to scroll delta in pixel coordinates. When the scale factor is non-integral, this conversion is not correct, since we clamp the result. I would prefer a better fix than the current patch :)

Would we be better off using some of the subpixel layout rounding functions Zalan created? Maybe if we took the ceiling of the calculated pixels we would always be drawing enough, without drawing too much?

For example, what if instead of "clampTo<int>" we used std::ceil?

Also, maybe we could adjust the clipRect like we do the scrollViewRect:

FloatRect rawClipRect(logicalClipRect);
rawClipRect.scale(scaleFactor);
FloatRect clipRect = enclosingIntRect(rawClipRect);

Or something like that?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #4)
> Comment on attachment 312426 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312426&action=review
> 
> >>> Source/WebKit/win/WebView.cpp:954
> >>> +        // FIXME: This is inefficient, we should be able to blit the scroll rectangle in this case as well.
> >> 
> >> Is this due to a bug in the CA accelerated compositing code path? Or is there a bug in BitBlt in the Windows code?
> > 
> > I believe the problem is the conversion from logical scroll delta to scroll delta in pixel coordinates. When the scale factor is non-integral, this conversion is not correct, since we clamp the result. I would prefer a better fix than the current patch :)
> 
> Would we be better off using some of the subpixel layout rounding functions
> Zalan created? Maybe if we took the ceiling of the calculated pixels we
> would always be drawing enough, without drawing too much?
> 
> For example, what if instead of "clampTo<int>" we used std::ceil?
> 
> Also, maybe we could adjust the clipRect like we do the scrollViewRect:
> 
> FloatRect rawClipRect(logicalClipRect);
> rawClipRect.scale(scaleFactor);
> FloatRect clipRect = enclosingIntRect(rawClipRect);
> 
> Or something like that?

Thank you, Brent! I will try out these ideas.

[Brent Fulgham]

Comment on attachment 312426 [details]
Patch

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

>>>>> Source/WebKit/win/WebView.cpp:954
>>>>> +        // FIXME: This is inefficient, we should be able to blit the scroll rectangle in this case as well.
>>>> 
>>>> Is this due to a bug in the CA accelerated compositing code path? Or is there a bug in BitBlt in the Windows code?
>>> 
>>> I believe the problem is the conversion from logical scroll delta to scroll delta in pixel coordinates. When the scale factor is non-integral, this conversion is not correct, since we clamp the result. I would prefer a better fix than the current patch :)
>> 
>> Would we be better off using some of the subpixel layout rounding functions Zalan created? Maybe if we took the ceiling of the calculated pixels we would always be drawing enough, without drawing too much?
>> 
>> For example, what if instead of "clampTo<int>" we used std::ceil?
>> 
>> Also, maybe we could adjust the clipRect like we do the scrollViewRect:
>> 
>> FloatRect rawClipRect(logicalClipRect);
>> rawClipRect.scale(scaleFactor);
>> FloatRect clipRect = enclosingIntRect(rawClipRect);
>> 
>> Or something like that?
> 
> Thank you, Brent! I will try out these ideas.

Did any of those ideas work?

[Brent Fulgham]

Comment on attachment 312426 [details]
Patch

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

>>>>>> Source/WebKit/win/WebView.cpp:954
>>>>>> +        // FIXME: This is inefficient, we should be able to blit the scroll rectangle in this case as well.
>>>>> 
>>>>> Is this due to a bug in the CA accelerated compositing code path? Or is there a bug in BitBlt in the Windows code?
>>>> 
>>>> I believe the problem is the conversion from logical scroll delta to scroll delta in pixel coordinates. When the scale factor is non-integral, this conversion is not correct, since we clamp the result. I would prefer a better fix than the current patch :)
>>> 
>>> Would we be better off using some of the subpixel layout rounding functions Zalan created? Maybe if we took the ceiling of the calculated pixels we would always be drawing enough, without drawing too much?
>>> 
>>> For example, what if instead of "clampTo<int>" we used std::ceil?
>>> 
>>> Also, maybe we could adjust the clipRect like we do the scrollViewRect:
>>> 
>>> FloatRect rawClipRect(logicalClipRect);
>>> rawClipRect.scale(scaleFactor);
>>> FloatRect clipRect = enclosingIntRect(rawClipRect);
>>> 
>>> Or something like that?
>> 
>> Thank you, Brent! I will try out these ideas.
> 
> Did any of those ideas work?

I spoke with Per Arne in person. It doesn't look like the other ideas work, so we should proceed with this for now.

Can you please change the FIME to reference a bugzilla that documents the need to improve the logic for this case?

[Per Arne Vollan]

Created attachment 359395 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #7)
> Comment on attachment 312426 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312426&action=review
> 
> >>>>>> Source/WebKit/win/WebView.cpp:954
> >>>>>> +        // FIXME: This is inefficient, we should be able to blit the scroll rectangle in this case as well.
> >>>>> 
> >>>>> Is this due to a bug in the CA accelerated compositing code path? Or is there a bug in BitBlt in the Windows code?
> >>>> 
> >>>> I believe the problem is the conversion from logical scroll delta to scroll delta in pixel coordinates. When the scale factor is non-integral, this conversion is not correct, since we clamp the result. I would prefer a better fix than the current patch :)
> >>> 
> >>> Would we be better off using some of the subpixel layout rounding functions Zalan created? Maybe if we took the ceiling of the calculated pixels we would always be drawing enough, without drawing too much?
> >>> 
> >>> For example, what if instead of "clampTo<int>" we used std::ceil?
> >>> 
> >>> Also, maybe we could adjust the clipRect like we do the scrollViewRect:
> >>> 
> >>> FloatRect rawClipRect(logicalClipRect);
> >>> rawClipRect.scale(scaleFactor);
> >>> FloatRect clipRect = enclosingIntRect(rawClipRect);
> >>> 
> >>> Or something like that?
> >> 
> >> Thank you, Brent! I will try out these ideas.
> > 
> > Did any of those ideas work?
> 
> I spoke with Per Arne in person. It doesn't look like the other ideas work,
> so we should proceed with this for now.
> 
> Can you please change the FIME to reference a bugzilla that documents the
> need to improve the logic for this case?

I have filed https://bugs.webkit.org/show_bug.cgi?id=193542. Thanks for reviewing!

[Per Arne Vollan]

rdar://problem/45269953

[WebKit Commit Bot]

Comment on attachment 359395 [details]
Patch

Clearing flags on attachment: 359395

Committed r240131: <https://trac.webkit.org/changeset/240131>

[Ahmad Saleem]

Landed: https://github.com/WebKit/WebKit/commit/148c3c85ecc0fbdbfbc3016c8850dd53585fd215