[prev in list] [next in list] [prev in thread] [next in thread]
List: koffice-devel
Subject: Re: Review Request: Change from asynchronous to synchronous adding of
From: "Thomas Zander" <zander () kde ! org>
Date: 2010-06-21 13:56:23
Message-ID: 20100621135623.14351.25788 () localhost
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On 2010-06-09 20:04:44, Thomas Zander wrote:
> > The description of the problem makes one assumtion that is incorrect; =
> > > The problem is that the update(pixelsF) three lines bellow that =
> > > triggers a paintEvent(...) which is handled right away
> > =
> > this is not correct, thats not how Qt works.
> > =
> > I don't know the problem you describe but the analyse is incorrect and =
replacing the singelshot with a direct call is also counter to the way that=
Qt works. Long running tasks should never be done in a paint event.
> > =
> > Maybe you can do a bit more research why you hit this issue you describ=
e. I wild guess might be that the cache is full. Is this on an embedded de=
vice? See the image cache API docs for more details.
> =
> Miroslav Nohaj wrote:
> The assumption might not be completely correct - it doesn't matter if=
the update() is handled right away or it just has higher priority than QTi=
mer or whatever, the point is that the QTimer will not fire as long as ther=
e are many update() calls. As I said - I could add another flag/condition t=
o PictureShape to not issue many updates when the image is not resized yet =
(because the QTimer has not fired yet) and thus allow the QTimer to fire an=
d resize the image, but that might be considered as workaround too.
> =
> To show that it does what I say I made a small .diff (which just adds=
4 debug strings - when the update() is called, when QTimer::singleShot() i=
s called, when the RenderQueue::renderImage() is called and when RenderQueu=
e::renderImage() really inserts the resized picture into the QPixmapCache) =
and you can take a look at the resulting debug strings to see what is reall=
y called and done.
> =
> So I just added the 4 debug strings: http://pastebin.com/vBAPjviB
> When you to zoom in, it looks like this (OK): http://pastebin.com/bH7=
547VY
> And when you zoom out, it looks like that (problem): http://pastebin.=
com/TAdwFj5q
> =
> When you do zoom out to a size which is not yet in QPixmapCache - wil=
l request many QTimer::singleShot, will not resize the picture and thus the=
picture will not be shown. After zoom in that picture will be shown, there=
will be no more calls to update() and QTimer::singleShot, and finaly the R=
enderQueue::renderImage() will be called (as requested by QTimer::singleSho=
t), the resized image will be put in the QPixmapCache, and there will be as=
many calls of RenderQueue::renderImage() after that as there was the count=
of calls to QTimer::singleShot.
> =
> I believe that calling the method directly is not wrong as it avoids =
lots calls of QTimer::singleShot and then lots of QTimer firing. But callin=
g update() (PictureShape.cpp:145) from paintEvent() is not a good idea - th=
e Qt manual says this: 'you should refrain from calling update() or repaint=
() inside a paintEvent()'.
> =
> I propose that if you want to draw as fast as possible and want to re=
size image independently on that, a thread might be a better solution then =
a timer.
>
Sounds like r1101954 should be reverted as that triggers your bug; I agree =
that update() should not be called at all in the paintEvent so the fix in 1=
101954 is wrong.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4262/#review6065
-----------------------------------------------------------
On 2010-06-09 12:25:14, Miroslav Nohaj wrote:
> =
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4262/
> -----------------------------------------------------------
> =
> (Updated 2010-06-09 12:25:14)
> =
> =
> Review request for KOffice.
> =
> =
> Summary
> -------
> =
> In some documents if you do page zoom out, some pictures are not drawn an=
d the KWord will hang in endless loop eating up all CPU time, but this happ=
ens only if you zoom out to a level that you didn't use before during this =
run of KWord with that document. This is because the PictureShape tries to =
access the resized image which is not yet in the QPixmapCache and because i=
t's not there, it asks for the image resizing triggered by a timer - QTimer=
::singleShot(0, m_renderQueue, SLOT(renderImage())). The problem is that th=
e update(pixelsF) three lines bellow that triggers a paintEvent(...) which =
is handled right away and will do the same as described in the previous sen=
tence, thus the QTimer will not fire the singleShot, the image will not get=
resized and this repeats until you do zoom in - then it will find a resize=
d image, the loop will end and the QTimer will finaly fire singleShot and t=
he image will get resized for the previous zoom level, thus when you do zoo=
m out again, it will not loop for that zoom level (but will as you do zoom =
out once agian to a new level).
> =
> The change just does replace delayed call of renderImage() by QTimer to a=
direct call. =
> =
> The options how can it be done are:
> - change asynchronous call of renderImage() to synchronous call (what I d=
id)
> - add some more logic to QCoreApplication::sendEvent, KApplication::notif=
y, QObject::event to make the QSingleShotTimer::timerEvent go through even =
though there are 'more important' things to handle, like update() requests
> - add flag in the picture shape to avoid unnecessary calls of update() if=
it was called before and we're just waiting for the image resize
> =
> ...and probably some more options how to do it. Please give comments if y=
ou would prefer one over the other.
> =
> =
> Diffs
> -----
> =
> /trunk/koffice/plugins/pictureshape/PictureShape.cpp 1136187 =
> =
> Diff: http://reviewboard.kde.org/r/4262/diff
> =
> =
> Testing
> -------
> =
> =
> Thanks,
> =
> Miroslav
> =
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://reviewboard.kde.org/r/4262/">http://reviewboard.kde.org/r/4262/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On June 9th, 2010, 8:04 p.m., <b>Thomas Zander</b> \
wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; \
padding-left: 10px;"> <pre>The description of the problem makes one assumtion that \
is incorrect; > The problem is that the update(pixelsF) three lines bellow that
> triggers a paintEvent(...) which is handled right away
this is not correct, thats not how Qt works.
I don't know the problem you describe but the analyse is incorrect and replacing \
the singelshot with a direct call is also counter to the way that Qt works. Long \
running tasks should never be done in a paint event.
Maybe you can do a bit more research why you hit this issue you describe. I wild \
guess might be that the cache is full. Is this on an embedded device? See the image \
cache API docs for more details.</pre> </blockquote>
<p>On June 21st, 2010, 12:08 p.m., <b>Miroslav Nohaj</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre>The assumption might not be completely correct - it doesn't matter \
if the update() is handled right away or it just has higher priority than QTimer or \
whatever, the point is that the QTimer will not fire as long as there are many \
update() calls. As I said - I could add another flag/condition to PictureShape to not \
issue many updates when the image is not resized yet (because the QTimer has not \
fired yet) and thus allow the QTimer to fire and resize the image, but that might be \
considered as workaround too.
To show that it does what I say I made a small .diff (which just adds 4 debug strings \
- when the update() is called, when QTimer::singleShot() is called, when the \
RenderQueue::renderImage() is called and when RenderQueue::renderImage() really \
inserts the resized picture into the QPixmapCache) and you can take a look at the \
resulting debug strings to see what is really called and done.
So I just added the 4 debug strings: http://pastebin.com/vBAPjviB
When you to zoom in, it looks like this (OK): http://pastebin.com/bH7547VY
And when you zoom out, it looks like that (problem): http://pastebin.com/TAdwFj5q
When you do zoom out to a size which is not yet in QPixmapCache - will request many \
QTimer::singleShot, will not resize the picture and thus the picture will not be \
shown. After zoom in that picture will be shown, there will be no more calls to \
update() and QTimer::singleShot, and finaly the RenderQueue::renderImage() will be \
called (as requested by QTimer::singleShot), the resized image will be put in the \
QPixmapCache, and there will be as many calls of RenderQueue::renderImage() after \
that as there was the count of calls to QTimer::singleShot.
I believe that calling the method directly is not wrong as it avoids lots calls of \
QTimer::singleShot and then lots of QTimer firing. But calling update() \
(PictureShape.cpp:145) from paintEvent() is not a good idea - the Qt manual says \
this: 'you should refrain from calling update() or repaint() inside a \
paintEvent()'.
I propose that if you want to draw as fast as possible and want to resize image \
independently on that, a thread might be a better solution then a timer. </pre>
</blockquote>
</blockquote>
<pre>Sounds like r1101954 should be reverted as that triggers your bug; I agree that \
update() should not be called at all in the paintEvent so the fix in 1101954 is \
wrong.</pre> <br />
<p>- Thomas</p>
<br />
<p>On June 9th, 2010, 12:25 p.m., Miroslav Nohaj wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for KOffice.</div>
<div>By Miroslav Nohaj.</div>
<p style="color: grey;"><i>Updated 2010-06-09 12:25:14</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0;">In some documents if you do page zoom out, \
some pictures are not drawn and the KWord will hang in endless loop eating up all CPU \
time, but this happens only if you zoom out to a level that you didn't use before \
during this run of KWord with that document. This is because the PictureShape tries \
to access the resized image which is not yet in the QPixmapCache and because it's \
not there, it asks for the image resizing triggered by a timer - \
QTimer::singleShot(0, m_renderQueue, SLOT(renderImage())). The problem is that the \
update(pixelsF) three lines bellow that triggers a paintEvent(...) which is handled \
right away and will do the same as described in the previous sentence, thus the \
QTimer will not fire the singleShot, the image will not get resized and this repeats \
until you do zoom in - then it will find a resized image, the loop will end and the \
QTimer will finaly fire singleShot and the image will get resized for the previous \
zoom level, thus when you do zoom out again, it will not loop for that zoom level \
(but will as you do zoom out once agian to a new level).
The change just does replace delayed call of renderImage() by QTimer to a direct \
call.
The options how can it be done are:
- change asynchronous call of renderImage() to synchronous call (what I did)
- add some more logic to QCoreApplication::sendEvent, KApplication::notify, \
QObject::event to make the QSingleShotTimer::timerEvent go through even though there \
are 'more important' things to handle, like update() requests
- add flag in the picture shape to avoid unnecessary calls of update() if it was \
called before and we're just waiting for the image resize
...and probably some more options how to do it. Please give comments if you would \
prefer one over the other.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/koffice/plugins/pictureshape/PictureShape.cpp <span style="color: \
grey">(1136187)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4262/diff/" style="margin-left: 3em;">View \
Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic