debugging memory issues

[Ant]

The fix doesn't break anything (still can't get it to crash without the fix), so I've added it to the SVN code.  Thanks again!

[john]

Quoting from me...

> what's to stop the destLine being beyond the end of the buffer ?

Thought about this some more and I don't think this will happen.
I'll try to explain myself.

In my case the visible rect values were (0,0,256,192). This is the initial position/size of the hello world screen. I was trying to drag the screen down by 1 pixel row. Screen::drag() does this by copying each row of pixels down to the row below, starting at the bottom of the screen and working back up to the top. You calculate the initial source and dest line positions correctly. The initial destLinei is the bottom pixel row, and the initial srcLinei is is the row above. After each row is copied the pointers are moved up 1 row. So, you need to repeat the DMA_Copy() 191 times (not 192), otherwise the source pointer goes 1 row above the top of frame buffer. Dragging by a larger amount goes even further off the top of the buffer. Hence my crash and the fix.

That's dragging down sorted, but I think dragging up might have a similar issue - although it almost certainly won't cause a crash this time, just garbage on the screen for an instant. Let's say for example that I've dragged the hello world screen down by one pixel and now I want to drag it back up by 1 pixel. At this point the visible rect height should be 191 (not 192). The initial destLinei needs to point to the top row and the initial srcLinei should be one row below it. Looks like you calculate these correctly. You work your way down the screen copying each row to the row above, however if you do this 191 times (the visible rect height), you'll copy 1 row from beyond the bottom of the screen onto the bottom row. This is not a big deal because Woopsi will re-draw the exposed region, however for the sake of completeness, I think the dragging up loop should look like this:

   for (u8 i = 0; i < visibleRects->at(0).height + vY; i++) {

Note the + this time, because vY is negative when dragging up.
(Am I being picky ?)

I don't know why the crash is so elusive though. On one machine it happens all the time (VC++ debugger says "bad ptr"), on the other, never. I thought that maybe if you try to drag the screen further down in one go (i.e. click down, then move the mouse downwards as quick as poss.) then maybe you'd get a bigger overshoot off the top of the buffer and increase the chances of a crash, but in doing this I've found another problem - possibly more to do with exposing regions. If I drag the screen down and up, down and up etc as if I'm trying to bang it on the top of the DS screen, I end up with a garbled screen. Does this happen for you too ?

Haven't looked into this one yet.

[Ant]

The garbled screen thing occurs because Windows tracks the mouse even if it is outside the region of the SDL window.  If you drag down fast enough, the screen will get dragged out of the SDL window entirely, and when you drag it back up bits of it are missing.  This isn't a problem on the DS, since it's impossible to drag the stylus outside of the physical display, so I'm happy to leave that bug in there.

[john]

Not sure I'm entirely convinced from the behavior I'm seeing.
Even if I drag the screen upwards slowly 1 pixel at a time, keeping the mouse within the SDL_app window, the bottom row of pixels doesn't get re-drawn as I'd expect it to, so I get this sort of vertical smudge effect (is there any way to post images to this forum?).
I've spent a bit of time debugging this, and when dragging up the screen by 1 pixel, at the end of Screen::drag() I can see it set up the new rect correctly...
rect = {x=0 y=191 width=256, height=1}
(i.e. the bottom row of pixels)
it then calls ((Woopsi*)_parent)->eraseRect(rect) and if I follow it through I can see it calling draw(Rect) on what seems like all of the right gadgets with the correct clipRect, but it doesn't seem to draw anything on the bottom row! Don't understand to be honest.

Anyway, having said all of that I'm happy to live with this bug too. The original crash was a real pain because it's easy to accidentally drag when you click - which is what was happening to me.

It's just so nice to be able to use a debugger! And now that I'm starting to get used to VC++, I'm quite impressed with it.

[Ant]

The vertical smudging is caused by your + vY fix, I'm afraid.  Remove the vY part and the smudging disappears.

If the screen is dragged up by 1 pixel, vY = -1.  So, the loop then says:

for (u8 i = 0; i < visibleRects->at(0).height + -1; i++) {

Which means the last row of pixels won't get copied.  The loop needs to be changed to this:

for (u8 i = 0; i < visibleRects->at(0).height; i++) {

When the screen is dragged down, it loses height, so subtracting vY is correct - we're copying fewer pixels than are displayed.  However, when we drag back up, all of the pixels on the screen *stay* on the screen (the additional ones get drawn back in later).

I've fixed the SVN version; see if that works!

VC++ is brilliant.  Visual Studio is, anyway; I can't vouch for the capabilities of MS' C++ compiler as I use gcc instead.  Best IDE I've ever used.

[john]

 
> Remove the vY part and the smudging disappears.
So it does.
But I still don't understand...

> for (u8 i = 0; i < visibleRects->at(0).height + -1; i++) {
> Which means the last row of pixels won't get copied.

Yes, but isn't that the idea? When dragging up by one pixel, you *don't* want to copy the last row of pixels.
You want to leave the last row of pixels as is (smudged) and then re-draw it. That's why you build the invalidated rect at the end of Screen::drag() and then call the eraseRect(). Afterall, if you copy the last row of pixels, where would you be copying it from ? There's no row below the bottom row.

But yes, without the +vY the smudging goes away and I'm just left with the drag issue you explained when the mouse goes outside the window. So apart from not understanding the above, I'm happy with what I've got - thankyou!

[Ant]

Imagine I have this situation:

Code:

xxxxxx
------

If I copy both rows up I get this situation:

Code:

xxxxxx
------
------

Now I overwrite the bottom row with new data:

Code:

xxxxxx
------
iiiiii

However, if I don't copy the last row, I get this:

Code:

xxxxxx
xxxxxx
------

I have two rows of 'x' because I didn't copy the '-' up.  Now when I overwrite the last row:

Code:

xxxxxx
xxxxxx
iiiiii

I've lost the row of dashes and kept the duplicate row of x.  If I drag up by another pixel, I get this:

Code:

xxxxxx
xxxxxx
xxxxxx
iiiiii

And when I overwrite the last row with something else:

Code:

xxxxxx
xxxxxx
xxxxxx
======

I've now lost the row of 'i' as well as the row of '-'.

[john]

Yep I see it now. Thanks for the explanation.
Excuse me while I give myself a good kicking...

There. That's better.

Seems obvious now... if the screen is dragged up you need to do as
many row copies as are currently visible of the screen.
Couldn't get my head around why the scroll down needed the adjustment
but the scroll up didn't.

[john]

Right, onto the next thing then (assuming I've not lost all credibility).
If you drag the screen halfway down and then quickly drag it up so the mouse
is about halfway up the top screen (but not outside the SDL window)
you can get parts of the top screen to start being dragged upwards,
accompanied by various amounts of smudging on both the top and bottom screens.
As you've previously described it's because with SDL it's possible to
get mouse locations which are outside the bottom screen, and so drag()
which calculates the screen number based on the mouse position sometimes
thinks it's working with screenNumber 1.

A very simple fix is to just ignore the drag event if it's not on screen 0.
So, at the top of Screen::drag() and after ...
        u8 screenNumber = calculatePhysicalScreenNumber(y);
add...
#ifdef USING_SDL
        if (screenNumber != 0) return false;
#endif

This stops the screen getting messed up but doesn't feel right because
if you drag upwards too quickly you can sometimes leave the screen behind.
What seems to work even better is this...

#ifndef USING_SDL
        u8 screenNumber = calculatePhysicalScreenNumber(y);
#else
        u8 screenNumber = 0;
#endif

That's assuming there aren't any situations where you expect to be able to
drag Screen's on the top screen ?
What do you think ?

[Ant]

I'm a bit dubious about putting workarounds in the SDL code because they actually harm portability.  If I wanted to use Woopsi on another platform I'd need to strip out the dual screen code as there aren't any other platforms with two physical screens.  The more workarounds there are for quirks when simulating the DS, the more code needs stripping when I port it.

[jeff]

If I wanted to use Woopsi on another platform I'd need to strip out the ...

... special methods that are in the box for switching a Screen from the bottom to the top hardware screen, etc.

The current baseline Woopsi assumes there are two distinct hardware screens - its not that tragic that the SDL code assumes the same and emulates them properly.

[Ant]

Mmm, fair point.  To be honest, the SDL stuff was a quick hack, but it I don't mind expending a bit more effort on it to make it better.  Especially as John's already done the work for me!  I've added the code in.

[john]

Hello! (Back from hols.) I'm wasn't ever suggesting a full SDL port. Just, this last couple of cases (the crash and the drag issue) were a couple of obvious ones that I found hard to ignore - they made me lose a little confidence in my debugging platform so I felt I had to investigate. I won't (intentionally) go hunting for more. Anyway, glad to see you've included the fix.