All Projects

The dircache shuts down (resets dircache_initialized to false) when it encounters files it does not know about, assuming a dircache inconsistency. One of the reasons for shutting down the dircache is when it encounters a file handle it doesn’t know about. To prevent this from happening, the dircache maintains a mapping between file handles and file names, which is maintained even when the dircache is not fully operating yet during the boot phase (by way of a small temporary cache array).

The problem is that this scheme has a few holes that allow files to be open without the dircache knowing about it:
1. When dircache_initializing is not set, the temporary cache is not maintained and newly opened files will slip through.
2. When the dircache is disabled and later re-enabled, open-file mappings that were active during the first dircache instance are not carried over to the new instance.

Both cases occur frequently when reloading the dircache after updating the database, which defaults to disabling the dircache and using the dircache buffer for its commit phase, and then reloading the dircache.
The most common case I see is when playback is active during the automatic DB update (Resume Playback is my startup screen). When I wait for the DB update and dircache reload to complete and then start a new playlist from the database, the playlist-control file (I believe) is closed. The new dircache instance does not know that this file was open before and shuts down (in update_filesize). (With current SVN, this later causes the DB browser to crash, but that’s a different story.)

I’m wondering what the best way to keep the dircache online would be. Some ideas:

* When the dircache shuts down, save the current list of bindings to the temporary cache, to be reloaded when the new instance starts up. Also set dircache_initializing = true, and reset it only when the temporary cache overflows). This would require a few more entries in the temporary cache than the current two.

* Recover the bindings from the FAT filesystem layer, or maybe even generally store them there. This layer already has a list of open files, but currently does not store a file-descriptor binding. Unfortunately, this solution does not work for hosted platforms.

* Make the dircache resilient against unknown file-descriptor bindings. It’s somewhat unclear to me how that could work, as each handle-based update that we ignore and just pass through to the filesystem creates a dircache inconsistency.

Thoughts?

I get the following build error:

CC apps/pcmbuf.c
/home/mhohmuth/priv/src/rockbox/rockbox/apps/pcmbuf.c: In function ‘pcmbuf_fade’:
/home/mhohmuth/priv/src/rockbox/rockbox/apps/pcmbuf.c:1122:9: warning: implicit declaration of function ‘thread_set_priority’
LD rockboxui
/home/mhohmuth/priv/src/rockbox/rockbox/build.sim/apps/pcmbuf.o: In function `pcmbuf_fade’:
/home/mhohmuth/priv/src/rockbox/rockbox/apps/pcmbuf.c:1122: undefined reference to `thread_set_priority’ /home/mhohmuth/priv/src/rockbox/rockbox/apps/pcmbuf.c:1143: undefined reference to `thread_set_priority’ collect2: ld returned 1 exit status

The offending source-code lines were introduced as part of the audio-mixer commit, r30097.

In a simulator build directory set up with “../tools/configure –sdl-threads”, a “make reconf” forgets the –sdl-threads setting and configures the build to use sigaltstack threads instead.

This patch set (in patch 0006) adds support to the tagnavi.config syntax for using a file’s basename (path name without directories) in display formats and conditions.

Patch 0007 changes the database’s default title format to display a file’s basename rather than “<Untagged>” if no title tag is present in the file.

In passing, patch 0001 fixes two bugs introduced with r26192 ( FS#10976  - make the <Untagged> string translatable):

* The translation was done too early: before the tagnavi format clauses were checked. This prevented comparisons against “<Untagged>” when a non-English translation was active.
* The code overwrote the buffer containing the constant string “<Untagged>” with the translation. This is neither safe nor necessary: The result pointer simply can be swapped with a pointer to the translation.

Patches 0002–0005 contain various minor DB cleanups the other patches rely on. (I’ll commit the fixes and cleanups separately from the basename feature after some testing.)

Patches relative to r29928.

The database has a feature that generates a track number from a track’s file name in case that information is missing from the tags. This track number is used solely in the database for displaying and sorting views displaying collections of tracks (such as album views).

The track number is assumed to be the last pair of digits in the file’s basename (sans the filename extension after the last “.”). I find this heuristic rather problematic for two reasons: Given there’s no track-number tag, (1) it assigns spurious track numbers for titles including multiple digits, and (2) typically I’d expect the track number to appear at the beginning of the filename, not at its end.

The attached experimental patch removes the track-number generation feature entirely. In result, tracks without a track-number tag are displayed without a track number and alphanumerically sorted by track title (not filename!).

This has the following effects:

* For tracks that do have a track number in their tags, nothing changes.

* For tracks that have no track number in either the tags or the file name, no spurious track number is generated and used for displaying / sorting. These tracks are now sorted by track title (the tagnavi.config default).

* For tracks that do have a track number in their file name (but not in their tags), this information is not used any longer and the tracks are now sorted by track title as well.

Arguably, tracks without a track-number tag by default could be sorted by the pathname of files, which often are organized and named in the intended track order. But this could be confusing as well and seems like just a another heuristic that’s prone to break. Also, users can use the file browser or define their own filename-based %format sort in tagnavi.conf. I’d argue against making this the default for DB views.

Please comment on which behavior you find most desirable. Thanks!

This patch adds a logical-or operator (”|”) for tagnavi conditionals.

Logical-and (”&”) always takes precedence over logical-or. (Brackets are not supported.)

Example:

“Unfinished Podcasts” → album ? filename ^ “/<microSD1>/PODCASTS” & playcount == “0” | filename ^ “/<microSD1>/PODCASTS” & lastoffset > “0” → title = “fmt_title”

“langtool.pl –deprecate” does not deprecate strings correctly. Currently, it replaces each string with “none” (without the quotes) and deletes value of the “user:” attribute. Doing it this way effectively removes the string ID and renumbers all subsequent IDs. This definitely is wrong for the english.lang (master) file, as it breaks voice files and compiled translations. (I’m not sure whether it’s a good thing to do for any other lang file.)

As per IRC wisdom, the correct way to deprecate a lang ID seems to be to change each string to “”“” (the double-quoted empty string). In this case, genlang also requires a value in the “user:” attribute, so I suggest not deleting this value.

(Apparently, changing the strings to “deprecated” used to be supported as well, but that is currently rejected by genlang.)

Enabling Rockbox USB on the ClipV2 (with attached patch 0001) has been very stable for me recently (since r29146, to be exact). I also do not need the hard-reset workaround from  FS#11664 .

But… for reasons I do not understand, r29169 (AMSv2 SD: Fix card insert lockups in USB mode; by jhMikeS) breaks USB for me, and I had to back this change out. Interestingly, the code in question addresses an SD-card issue, but the ClipV2 does not have an SD-card slot.

The symptom is occasional SCSI errors on my Linux host trying to read certain sectors. fsck.vfat fails with an I/O error attempting to read a high-numbered sector (possibly the MBR backup sector).

For the convenience of users who’d like to try Rockbox USB on the ClipV2, the attached patches enable USB (patch 0001) and revert r29169 (patch 0002).

Rockbox on my ClipV2 cannot play back (at least certain) MPEG-4 audio files. When these files are encountered in the playlist, playback skips over them. In that instant I can see that the track metadata is displayed correctly in the WPS, and it is also correct in the database.

Sometimes skipping over the “broken” tracks even hangs playback totally, with a subsequent non-mp4 file stuck at 0:00, requiring a stop + resume.
Sometimes, I see a “codec error” splash for a couple of milliseconds when the files are skipped.

I have tested various revisions between SVN current and r28308 (a pre-3.7 revision), and all show the same behavior.

Here are two files I’ve encountered this problem with: (warning, large podcast tracks)

http://www.club-autonomic.com/podcast/autonomic_podcast_layer_11.m4a
http://www.club-autonomic.com/podcast/autonomic_podcast_layer_12.m4a

These files seem to play fine in the simulator.

However, one really strange thing I’ve encountered is that the simulator pauses playback after playing back the file for a few minutes. I can then manually unpause to resume playback of the track. This may be a related or totally unrelated problem.

This mailing-list report may be related:

http://www.rockbox.org/mail/archive/rockbox-archive-2011-03/0011.shtml

Observed with r29143 with enabled USB driver ( FS#11664 ):

During an USB transfer, my ClipV2 shows:

*PANIC*
Unhandled masked IRQ 03: INT_USB (status 0x00000008).

I suspect that r29130 is to blame. I'll run without this patch for a few days and see what happens.

Here are fixes for various whitespace errors I’ve encountered in my journeys through the source code. I find the first one especially nasty. The second one is rather benign, the third one is somewhat misleading.

Patches relative to r29071 attached.

Better version reporting for git-tracked repositories: Include the git commit ID in the version string, much like is done for repos tracked with bzr.

SVN r29045

firmware/powermgmt.c:shutdown_hw() contains the following code snippet twice:

  while (storage_disk_is_active())
      sleep(HZ/10);

This looks bogus to me.

I recently enabled Rockbox-native USB slave mode on my Sansa ClipV2 through the patch provided in  FS#11664  - Workaround for usb random failures on amsv2. Ever since then, I have had very stable USB connections, but regular filesystem corruption on the player’s flash drive. This had never happened before, when I still was had to boot into the OF to use its USB support.

As I allow Rockbox to write to my disk for various reasons, such as scrobbler logging and database statistics updates. Also, my flash drive is fairly full, leading to frequent disk-block reallocation. Thus I suspected that either (1) the filesystem wasn’t fully flushed when USB slave mode was entered, or (2) that Rockbox was using stale filesystem data after returning from USB slave mode that would later lead to filesystem corruption. I slightly lean towards explanation 2, because I never observed filesystem corruption when powering off Rockbox (to use the OF’s USB support) even though the shutdown code does not unmount the disks either, AFAIK.

As an experiment, I added support for unmounting all disks before entering USB slave mode – see attached patch. I’ve been running with this patch for a week now, and I haven’t seen any filesystem corruption since then.

Comments?

Testers / comments wanted!

This experimental patch adds an automatic multi-resume feature that supports complex listening habits, such as skipping back and forth among multiple half-played tracks. It automatically remembers and restores a resume position for each track without user intervention, without relying on the bookmarking system and without requiring any special navigation; resuming works whichever way a track is found (file browser, database, playlist).

The feature is modeled after the podcast/audiobook resume feature of the Sansa ClipV2 / Clip+, IMHO the sole feature of the original Sansa firmware that was better than what Rockbox has. This implementation is more generic, though, and works for all tracks.

In summary, it works like this: Whenever playback of a track is stopped, the current playback offset is stored in the tag database. (The offset is reset if a track is played all the way to its end.) The offset is restored whenever a track is played back again, except for short tracks in case of an automatic song change.

This patch is experimental as it hard-codes some of the behavior and isn’t factored out terribly well. Although I have thoroughly tested the feature, I seem to never have triggered execution of some parts of the patch, especially the changes to tagcache_fill_tags and build_numeric_indices. I am requesting feedback on whether these changes make any sense at all.

Furthermore, this patch relies on a few bug fixes to the tag database (see  FS#11721 ,  FS#11722 , and  FS#11723 ). I am attaching to this report another patch that fixes all three bugs for your convenience.

Caveats:

* Only works if runtime statistics have been enabled. Ideally the resume feature would be independent from runtime statistics; perhaps it should be implemented as a separate consumer of the PLAYBACK_EVENT_TRACK_BUFFER and PLAYBACK_EVENT_TRACK_FINISH events.

* Always resumes tracks if possible (under the condition mentioned in the next caveat). If a resume is unwanted, the user can always press the “skip back” button to play the track from the beginning. This should be configurable (always/never/ask user).

* Hard-coded resume behavior: On automatic track change, the next track will be resumed at its resume position if the track is longer than 20 minutes (assuming it is a partially played podcast/audiobook); otherwise, the track will be started anew (assuming a mid-track playback stop for an album track). This feature and its threshold should be configurable.

The problem is that tagcache_update_numeric queues database updates using tagcache.c’s command_queue, whereas tagcache_get_numeric consults the database directly without consulting the command_queue. This can lead to lost updates when pending changes have not yet been processed and committed.

I was able to reproduce this problem with my Sansa ClipV2, but not with the simulator, presumably because of a different thread schedule due to my PC’s better horsepower.

To reproduce, I have been jumping back and forth between two tracks of a playlist. The following slightly edited logf output shows the buffer and unbuffer events (“be:”, “ube:”) logged by tagtree_buffer_event and tagtree_track_finish_event, respectively, along with the playcount read from and written to the tagcache by these functions:

be:/PODCASTS/foo.mp3
-> 3
[...]
ube:/PODCASTS/foo.mp3
-> 4
[...]
be:/PODCASTS/bar.mp3
-> 4
[...]
ube:/PODCASTS/bar.mp3
-> 5
[...]
be:/PODCASTS/foo.mp3
-> 3
[...]
ube:/PODCASTS/foo.mp3
-> 4
[...]
be:/PODCASTS/bar.mp3
-> 4
[...]
ube:/PODCASTS/bar.mp3
-> 5
[...]

As you can see, tagtree_track_finish_event increments the playcount each time, but the next tagtree_buffer_event for the same track reads the old, stale playcount again.

I have considered three solutions to this problem:

1. Flush the command queue in tagcache_get_numeric with a call to run_command_queue(true).

2. Update the in-memory database synchronously in tagcache_update_numeric, and queue only updates to the on-disk database in the command_queue.

3. Store-to-load forwarding: In tagcache_get_numeric, consult the command queue for uncommitted updates to the tag being read.

Solution 1, while simple, also is the most inefficient one as it delays all tagcache reads until all writes have been committed. Also, I encountered the occasional lockup during background database updates – not quote sure why, though.

I was able to implement Solution 2 after some refactoring. Unfortunately, this solution is incomplete: It depends on the in-memory database, and it can fail if the tag being updated has not been loaded into memory yet.

I ended up with Solution 3 (patch attached). It may not be the shortest solution, but it is the most elegant one, as it does not alter the tagcache’s update policy and is complete nonetheless.

(You may wonder why I am so interested in tagcache accuracy (see also  FS#11721  and  FS#11722 ) – after all, this is only statistics data? The answer is that I am working on another use case for the tag cache that requires accuracy: I am using the tagcache as the ultimate resume database, which allows resuming every track at its last-played position, independent of the navigation mechanism used to access the track (no file browser / bookmarking system needed).)

The reason is that, depending on the thread scheduling, the tagtree_buffer_event function may attempt to access the tagcache before it is initialized.

I could reproduce this problem on my Sansa ClipV2, but not with the simulator, which had a slightly different schedule during startup.

When problem occurs, the debug logfiles shows this:

load track: /PODCASTS/foo.mp3
codec < Q_CODEC_LOAD_DISK
be:/PODCASTS/foo.mp3
tc stat: not found: /PODCASTS/foo.mp3
[...]
loading tagcache to ram...
tagcache loaded into ram!
[...]
Request new track
codec >| audio Q_AUDIO_CHECK_NEW_TRACK
audio < Q_AUDIO_CHECK_NEW_TRACK
No tagcache index pointer found

As you can see, the tagcache is initialized after the first song is buffered. The “tc stat: not found” message is generated by tagtree_buffer_event, and the “No tagcache index pointer found” message is generated by tagtree_track_finish_event.

The attached patch fixes the problem by deferring the use of the tagcache until it has been initialized.

Runtime data gathering in tagtree_buffer_event / tagtree_track_finish_event (tagtree.cc) relies on the PLAYBACK_EVENT_TRACK_BUFFER and PLAYBACK_EVENT_TRACK_FINISH events. The latter event is generated only on a song change, but not when playback stops (user stops playback, poweroff, or end of playlist). This leads to statistics not being updated and old statistics data being loaded again next time a previously stopped song is played.

To reproduce, turn on logf and observe the “be:” (buffer event) and “ube:” (unbuffer event) messages in the debug log. Stopping playback does not generate a “ube:” message.

The attached patch fixes this behavior by generating the PLAYBACK_EVENT_TRACK_FINISH event from the audio_stop_playback function as well.