Re: Recenter video

Re: Recenter video

[Lennart Borgman]

2010/6/23 David Engster <deng@randomsample.de>:
> Lennart Borgman writes:
>> Could you please send me the file you are testing with so I can see if
>> this happens here with my patch? (It takes me a bit longer to send you
>> the patch since I have a lot of patches.)
>
> Sure. Have fun with my old diploma thesis. ;-)


Hi Eli and David,

I have now tested with the file you sent me, David. (It scrolls so
slow I can nearly read it though my German is nearly non-existent
now.)

I can see the "jumping scrolling", but the bug here is not the same as
the one I have tried to fix. Here I can see it enters the
"clear_glyph" path in try_scrolling and I think this is what Eli have
been working on.

I have not merged in Eli's changes yet, but now I think I can do that
since it is now clear we have been looking at two different reasons
for the jumping scrolling.

I do not think Eli's changes cure the problem I have seen (but I am
not sure) and my changes alone can't cure your problem (but it cured
the problem I saw when scrolling window.c).

Just to sum things up: We also still have what I called the bug around
 702 in window.c.

For me this was a good step forward. Thanks for sending the file.

- L

Re: Recenter video

[Eli Zaretskii]

> From: Lennart Borgman <lennart.borgman@gmail.com>
> Date: Wed, 23 Jun 2010 02:41:08 +0200
> Cc: 
> 
> I can see the "jumping scrolling", but the bug here is not the same as
> the one I have tried to fix. Here I can see it enters the
> "clear_glyph" path in try_scrolling and I think this is what Eli have
> been working on.

If you still don't use the latest changes on the trunk, it's a small
wonder that you see recentering inside try_scrolling.

> I do not think Eli's changes cure the problem I have seen (but I am
> not sure) and my changes alone can't cure your problem (but it cured
> the problem I saw when scrolling window.c).

The problem is that your changes are incorrect: they violate the
redisplay protocol that doesn't allow using the current glyph matrix
if the last redisplay cycle was paused (a.k.a. interrupted) because
more input arrived.  The window_end_valid flag being nil means that
the redisplay was not finished, and in that case we cannot use the
try_scrolling method.  So your changes could work in some special
cases, but using them means we will certainly see redisplay bugs in
the future.

The only way to fix this case correctly is either to force redisplay
finish with its job, at least for a single window, before it pauses,
or improve the code that sets the window_end_valid flag so that it
does not give up too easily.

But to make such changes reliably I need to reproduce the problem on
my machine.  Until now, I can only reproduce it extremely rarely (like
once in the whole scroll of window.c, and only sometimes), even if I
load my CPU with 2 programs each of which eats up 100% of CPU
resources...

Re: Recenter video

[Lennart Borgman]

On Wed, Jun 23, 2010 at 5:06 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Lennart Borgman <lennart.borgman@gmail.com>
>> Date: Wed, 23 Jun 2010 02:41:08 +0200
>> Cc:
>>
>> I can see the "jumping scrolling", but the bug here is not the same as
>> the one I have tried to fix. Here I can see it enters the
>> "clear_glyph" path in try_scrolling and I think this is what Eli have
>> been working on.
>
> If you still don't use the latest changes on the trunk, it's a small
> wonder that you see recentering inside try_scrolling.


What do you mean? What do you not expect to happen?


>> I do not think Eli's changes cure the problem I have seen (but I am
>> not sure) and my changes alone can't cure your problem (but it cured
>> the problem I saw when scrolling window.c).
>
> The problem is that your changes are incorrect: they violate the
> redisplay protocol that doesn't allow using the current glyph matrix
> if the last redisplay cycle was paused (a.k.a. interrupted) because
> more input arrived.  The window_end_valid flag being nil means that
> the redisplay was not finished, and in that case we cannot use the
> try_scrolling method.


I can't see that this happens because of my change. The test for
window_end_valid flag is still there with my patch. The only thing I
have changed is when clip_changed is set because of changes in
clipping. Everything else remains the same.

Can you please explain exactly what you mean?


> So your changes could work in some special
> cases, but using them means we will certainly see redisplay bugs in
> the future.
>
> The only way to fix this case correctly is either to force redisplay
> finish with its job, at least for a single window, before it pauses,
> or improve the code that sets the window_end_valid flag so that it
> does not give up too easily.
>
> But to make such changes reliably I need to reproduce the problem on
> my machine.  Until now, I can only reproduce it extremely rarely (like
> once in the whole scroll of window.c, and only sometimes), even if I
> load my CPU with 2 programs each of which eats up 100% of CPU
> resources...

Re: Recenter video

[Eli Zaretskii]

> From: Lennart Borgman <lennart.borgman@gmail.com>
> Date: Wed, 23 Jun 2010 11:44:33 +0200
> Cc: deng@randomsample.de, emacs-devel@gnu.org
> 
> > If you still don't use the latest changes on the trunk, it's a small
> > wonder that you see recentering inside try_scrolling.
> 
> 
> What do you mean? What do you not expect to happen?

I expect no more recentering when try_scrolling is called.

> > The problem is that your changes are incorrect: they violate the
> > redisplay protocol that doesn't allow using the current glyph matrix
> > if the last redisplay cycle was paused (a.k.a. interrupted) because
> > more input arrived.  The window_end_valid flag being nil means that
> > the redisplay was not finished, and in that case we cannot use the
> > try_scrolling method.
> 
> 
> I can't see that this happens because of my change. The test for
> window_end_valid flag is still there with my patch. The only thing I
> have changed is when clip_changed is set because of changes in
> clipping. Everything else remains the same.

Doesn't your patch cause try_scrolling to be called?  If so, that's a
no-no when the window_end_valid flag is nil.

Re: Recenter video

[Lennart Borgman]

On Wed, Jun 23, 2010 at 5:18 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Lennart Borgman <lennart.borgman@gmail.com>
>> Date: Wed, 23 Jun 2010 11:44:33 +0200
>> Cc: deng@randomsample.de, emacs-devel@gnu.org
>>
>> > If you still don't use the latest changes on the trunk, it's a small
>> > wonder that you see recentering inside try_scrolling.
>>
>>
>> What do you mean? What do you not expect to happen?
>
> I expect no more recentering when try_scrolling is called.


I have merged in your changes now. Something unfortunately seems to be wrong.

Without your changes (but with only my patch) I saw no "jumping
scrolling" in window.c when leaning on down arrow. With your changes
merged in I see now a few "jumping scrolls".

However I see nothing special in my debugging output so I do not know
why. Do you have any idea what I could look for?


>> > The problem is that your changes are incorrect: they violate the
>> > redisplay protocol that doesn't allow using the current glyph matrix
>> > if the last redisplay cycle was paused (a.k.a. interrupted) because
>> > more input arrived.  The window_end_valid flag being nil means that
>> > the redisplay was not finished, and in that case we cannot use the
>> > try_scrolling method.
>>
>>
>> I can't see that this happens because of my change. The test for
>> window_end_valid flag is still there with my patch. The only thing I
>> have changed is when clip_changed is set because of changes in
>> clipping. Everything else remains the same.
>
> Doesn't your patch cause try_scrolling to be called?  If so, that's a
> no-no when the window_end_valid flag is nil.


I don't think so, but since you suspect it I must ask you why you
think it does that.

The only change I am aware of in my patch is really that clip_changed
is set differently. (Anything else is a typo or similar.) So why do
you think this causes try_scrolling to be called? And when?

Re: Recenter video

[Lennart Borgman]

On Wed, Jun 23, 2010 at 6:14 PM, Lennart Borgman
<lennart.borgman@gmail.com> wrote:
> On Wed, Jun 23, 2010 at 5:18 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>>> From: Lennart Borgman <lennart.borgman@gmail.com>
>>> Date: Wed, 23 Jun 2010 11:44:33 +0200
>>> Cc: deng@randomsample.de, emacs-devel@gnu.org
>>>
>>> > If you still don't use the latest changes on the trunk, it's a small
>>> > wonder that you see recentering inside try_scrolling.
>>>
>>>
>>> What do you mean? What do you not expect to happen?
>>
>> I expect no more recentering when try_scrolling is called.
>
>
> I have merged in your changes now. Something unfortunately seems to be wrong.
>
> Without your changes (but with only my patch) I saw no "jumping
> scrolling" in window.c when leaning on down arrow. With your changes
> merged in I see now a few "jumping scrolls".

That was not quite correct. What I see now is:

- If I run with both our changes under the debugger I see some
"jumping scrolling" somewhere around line 300 in window.c.

- If I run without the debugger I do not see this..

> However I see nothing special in my debugging output so I do not know
> why. Do you have any idea what I could look for?

I still wonder this.

Re: Recenter video

[Eli Zaretskii]

> From: Lennart Borgman <lennart.borgman@gmail.com>
> Date: Wed, 23 Jun 2010 18:14:25 +0200
> Cc: deng@randomsample.de, emacs-devel@gnu.org
> 
> > Doesn't your patch cause try_scrolling to be called?  If so, that's a
> > no-no when the window_end_valid flag is nil.
> 
> 
> I don't think so, but since you suspect it I must ask you why you
> think it does that.

As far as I remember, the history of this was as follows:

You found that the clip_changed flag was the one that prevented
try_scrolling from being called, in this part of redisplay_window:

  /* Try to scroll by specified few lines.  */
  if ((scroll_conservatively
       || scroll_step
       || temp_scroll_step
       || NUMBERP (current_buffer->scroll_up_aggressively)
       || NUMBERP (current_buffer->scroll_down_aggressively))
      && !current_buffer->clip_changed
      && CHARPOS (startp) >= BEGV
      && CHARPOS (startp) <= ZV)
    {
      /* The function returns -1 if new fonts were loaded, 1 if
	 successful, 0 if not successful.  */
      int rc = try_scrolling (window, just_this_one_p,
			      scroll_conservatively,
			      scroll_step,
			      temp_scroll_step, last_line_misfit);

(When try_scrolling cannot be called, or when it is called and reports
a failure, Emacs falls back to recentering around point.)

The clip_changed flag was supposed to be reset by
reconsider_clip_changes, but it wasn't reset because of the condition
indicated below:

  if (b->clip_changed
	   && !NILP (w->window_end_valid)  <<<<<<<<<<<<<<<<<
	   && w->current_matrix->buffer == b
	   && w->current_matrix->zv == BUF_ZV (b)
	   && w->current_matrix->begv == BUF_BEGV (b))

Your patch tweaks the clip_changed flag so that it is now not set, and
the obstacle for calling try_scrolling is thereby removed.  But the
window_end_valid flag is still nil, because your patch does nothing to
change its value.  Calling try_scrolling when window_end_valid is nil
is not allowed, because try_scrolling reuses the current glyph matrix,
which is only guaranteed to be up to date when window_end_valid is
non-nil.

To check the correctness of my description above, please put a
breakpoint inside try_scrolling conditioned by w->window_end_valid
being equal to Qnil, then scroll by leaning on the down arrow, and see
if the breakpoint ever breaks.  If it does break, your patch violates
the redisplay protocol regarding reuse of the current matrix.

Re: Recenter video

[Eli Zaretskii]

> Date: Wed, 23 Jun 2010 06:06:32 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: deng@randomsample.de, emacs-devel@gnu.org
> 
> But to make such changes reliably I need to reproduce the problem on
> my machine.  Until now, I can only reproduce it extremely rarely (like
> once in the whole scroll of window.c, and only sometimes), even if I
> load my CPU with 2 programs each of which eats up 100% of CPU
> resources...

I found a way to artificially slow down redisplay, and now I can
reliably reproduce the recentering while scrolling window.c.  It
indeed happens because the window_end_valid flag is nil, and therefore
reconsider_clip_changes does not reset the clip_changed flag, which in
turn prevents try_scrolling from being called.

I can now look for a solution to this problem.  Thanks to all who
patiently tried what I asked them to and explained what they saw.