Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#1260 closed defect/bug (fixed)

Certain OSD items display garbage when resized

Reported by: mvglasow (2) Owned by: Singesang
Priority: major Milestone: version 0.5.1
Component: osd/core Version: git master
Severity: normal Keywords: bug, osd, resize, patches
Cc: http://wiki.navit-project.org/index.php/user:mvglasow, (2)

Description

This is a follow-up of #983.

#983, #990, #1098 and #1256 deal with relative (percentage) values for OSD size and position not being honored until the window is resized. These issues were fixed in r5901.

However, work on these tickets found another bug, which appears to have a different cause: Certain types of OSD items will render garbage if the display is resized.

The issue seems to affect only OSD items with relative size. OSD items with fixed size but relative position do not seem to be affected. Also, only certain OSD types have issues.

Behavior I observed so far is the following:

  • button – affected
  • compass – OK
  • navigation_next_turn – affected
  • odometer – OK
  • scale – OK (as per sleske's comment on #983).
  • stopwatch – OK
  • text – affected

Affected items display normally at first. Upon first resize, they will render either just the semitransparent background, or a black box, or a garbled clipping from nearby screen content (on Linux), occasionally with colors swapped. Size and position are OK, though.

Also, when the content of an affected item changes (e.g. new turn instruction in navigation_next_turn, new text displayed in text), that item will display normally again.

I've been looking through the code but none of my guesses at potential error sources so far have been confirmed.

There seems to be some difference in the draw callbacks upon creation or content update vs. redraw on resize (with content unchanged). Maybe some data gets deallocated after drawing and is thus no longer available on redraw, while a content update would recreate the data? Where would that happen in the code?

The original code was committed by tinloaf (who seems to be no longer active) between r2140–r2174.

If anyone has an idea, any input is welcome.

Attachments (2)

trac1260_pre1.patch (1.9 KB) - added by mvglasow (2) 5 years ago.
Crude fix for button redraw (works for use=overlay=1 only)
trac1260.patch (15.6 KB) - added by mvglasow (2) 5 years ago.
Updated patch, which also fixes toggle_announcer

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by mvglasow (2)

  • Cc http://wiki.navit-project.org/index.php/user:mvglasow (2) added

comment:2 Changed 5 years ago by mvglasow (2)

My last suspicion got confirmed: some data structure gets discarded after the inital draw and is not recreated properly before redraw.

I've managed to fix button (at least for cases in which use_overlay is set) by copying a whole bunch of code from osd_button_set_attr (the part for the src attribute) into osd_button_draw. With that, the button gets redrawn correctly upon resize.

Changed 5 years ago by mvglasow (2)

Crude fix for button redraw (works for use=overlay=1 only)

comment:3 Changed 5 years ago by mvglasow (2)

A preliminary patch is attached. Note that it's not a production-level patch but intended for review and input. It fixes just one aspect of the issue and is most likely quite ugly in a bunch of ways:

  • Duplicated code: The new code in osd_button_draw was copied from osd_set_attr and pasted with minimal modifications.
  • Resource efficiency: Essentially, we're discarding data and recreating the same data again – because I haven't found the place in which the data gets discarded. This could probably be resolved more nicely by keeping the data around so we can reuse it, instead of recreating it.

If someone knows their way around the OSD code better than I do: please have a look at the patch and tell me how I can achieve the same result more nicely.

Currently I've tackled only button. For the moment it appears that a bunch of OSD items all suffer from the same design flaw, though each has to be examined and fixed individually.

As for items affected: the bug seems to affect all items that use (or try to use) pre-rendered data. The items that are not affected are fairly simple and rendered from scratch each time.

comment:4 Changed 5 years ago by mvglasow (2)

After deeper investigation I found that there are multiple culprits involved:

  • Some OSDs (gps_status, navigation_next_turn, text) had a do_draw variable defined in their draw function, initially set to zero and changed only if content was found to have changed.
  • button and image (which share the same draw code) are not affected by the above but seem to refer to some already-dereferenced data when redrawing. This is addressed in the patch I mentioned earlier, but not necessarily in the most elegant manner.
  • auxmap seems to have different issues and will never produce anything useful at a relative size (haven't yet figured out why, this probably warrants another ticket).

I have solved the do_draw issue in the following manner:

  • Introduced a do_draw member into osd_item, which indicates that the item needs to be redrawn.
  • Set the do_draw member in osd_std_calculate_sizes_and_redraw and clear it after the draw callback returns
  • In the various draw methods, initialize the local do_draw variable with the item's do_draw member and eliminate all instances in which do_draw is set to zero.

This ensures every size recalculation triggers a full redraw.

Status is now:

  • auxmap: broken, probably a separate issue
  • button: fixed (PoC-grade)
  • compass: OK
  • cmd_interface: not tested, probably OK
  • gps_status: fixed (do_draw)
  • image: fixed (PoC-grade)
  • navigation_next_turn: fixed (do_draw)
  • odometer: OK
  • scale: probably OK
  • stopwatch: OK
  • text: fixed (do_draw)
  • toggle_announcer: OK (but separate issue, see #1261)

Not tested: route_guard, speed_cam, speed_warner, volume

comment:5 Changed 5 years ago by mvglasow (2)

As for osd_button_draw, I've thrown out some obviously unneeded duplicate code. For testing purposes I've inserted a printf() each time a buton gets redrawn.

I then started a burn-in test, configuring the OSD with four buttons with use_overlay="0", demo vehicle with follow="1", and sent it on a 1900-km trip from Frankfurt (Oder), Germany, to Barcelona. The printf() output indicates that the four buttons indeed get redrawn every second.

Currently, 16 hours into the test, the demo vehicle has just made it from one Frankfurt to the other (i.e. Frankfurt am Main), about 600 km. Virtual memory usage has been oscillating between 141M and 148M all the time.

This indicates that, despite any inefficiencies in the code, there does not seem to be any major memory leak.

comment:6 Changed 5 years ago by mvglasow (2)

Here's the patch, which fixes all of the OSD items listed as fixed in the previous comment. It supersedes the preliminary patch I posted earlier.

The four items listed as "not tested" were not changed. They never used the do_draw logic and are likely to be unaffected.

In the meantime, my demo vehicle is near Rastatt, about 750 km from its starting point, 1150 km to go. Virtual memory usage is still in the same range, currently at 146–147M.

comment:7 Changed 5 years ago by mvglasow (2)

  • Keywords patches added

comment:8 Changed 5 years ago by mvglasow (2)

Turns out toggle_announcer was also affected, but the issue was overshadowed by #1261. A new patch, which fixes toggle_announcer, including #1261, will follow shortly.

Changed 5 years ago by mvglasow (2)

Updated patch, which also fixes toggle_announcer

comment:9 Changed 5 years ago by mvglasow (2)

Update on auxmap: I created #1262 for the issue, as the symptoms differ from the ones described here

comment:10 Changed 5 years ago by mvglasow (2)

A patch is available for #1262. While #1260 and #1262 touch different pieces code, it is recommended to apply first #1260, then #1262.

comment:11 Changed 5 years ago by kazer

  • Resolution set to fixed
  • Status changed from new to closed

Thank you mvglasow, nice work here.

Patch applied in r5910

I replaced your printf with a dog(1)

comment:12 Changed 5 years ago by mvglasow (2)

Thanks for applying the patch. As for the printf() – that was really some of my debug code to figure out what was going on and which I forgot to take out. Feel free to remove it, unless you think it might be useful for something...

Note: See TracTickets for help on using tickets.