Bug 173152 - [Win][HighDPI] Repaint glitches when scrolling.
Summary: [Win][HighDPI] Repaint glitches when scrolling.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-09 04:41 PDT by Per Arne Vollan
Modified: 2019-09-05 20:44 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.15 KB, patch)
2017-06-09 04:43 PDT, Per Arne Vollan
bfulgham: review+
Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2019-01-17 11:26 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2017-06-09 04:41:07 PDT
This happens when the device scale factor is non-integral, e.g. 1.25x.
Comment 1 Per Arne Vollan 2017-06-09 04:43:07 PDT
Created attachment 312426 [details]
Patch
Comment 2 Brent Fulgham 2017-06-09 08:42:39 PDT
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?
Comment 3 Per Arne Vollan 2017-06-09 09:01:24 PDT
(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 :)
Comment 4 Brent Fulgham 2017-06-20 12:38:02 PDT
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?
Comment 5 Per Arne Vollan 2017-06-20 12:48:16 PDT
(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.
Comment 6 Brent Fulgham 2017-08-18 09:52:46 PDT
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?
Comment 7 Brent Fulgham 2019-01-17 09:17:28 PST
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?
Comment 8 Per Arne Vollan 2019-01-17 11:26:15 PST
Created attachment 359395 [details]
Patch
Comment 9 Per Arne Vollan 2019-01-17 13:20:24 PST
(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!
Comment 10 Per Arne Vollan 2019-01-17 13:22:39 PST
rdar://problem/45269953
Comment 11 WebKit Commit Bot 2019-01-17 13:45:13 PST
Comment on attachment 359395 [details]
Patch

Clearing flags on attachment: 359395

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