CachingReader block the player thread while it is caching

[Alban]

In the current implementation of CachingReader run() take the lock and process the whole queue of chunks to cache in a single go. If the queue is large enough this eventually block the playback thread because it need the lock to read data from the cache.

Ideally the CachingReader should be made such that reading access is non blocking as long as the data is in the cache. However as it mean a near complete re-write I first tried a workaround that can be found in the attached patch. It simply change the caching thread to release the lock and yield between each read. This give a chance
 to the playing thread to get at its data in a reasonable time.

I also changed the data type for the chunk queue from a QSet to a QList to make sure that the queued chunks are kept in the order they were inserted.

Additionally it should be noted that the ReadAheadManager request quiet a lot of data in advance, up to several seconds as far as i can tell. Combined with the above problem this lead to audio skips on every track load or seek with slower machines. So I also reduced the pre-read blocks from 64 to 4, again this is just a workaround, ideally the number of blocks should be computed to give a fix time amount.

This seems to be the problem behind bug #666052, and this patch allow me to use mixxx on my netbook again :)

[Owen Williams (ywwg)]

Even on my fast system this patch lets me cut the latency nearly in half. I'll keep testing, and Bill will want to take a look I'm sure, but this is very promising.

[William Good (bkgood)]

I unfortunately don't have anything capable of regularly triggering this anymore (yea this is a dupe of 666052 but that bug's kinda noisy so let's talk here) so I'm going off what other people tell me.

Breaking up the reads is good, imo. Excellent thinking there.

Changing the QSet to a QList: I'm guessing RJ used the set class for set functionality -- the structure explicitly prevents duplicates. Lists allow duplicates. How important is it that chunks are read from disk in the order they are requested? What's the general range of cardinalities/lengths of the set/list under general operation?

Regarding the RAMAN change, did you try multiple values for n or just 4? I'd be interested to know how high n can be before problems begin to occur (given a netbook is probably the least powerful class of systems we'd want to really try to support, this seems like useful info.).

Thanks for your work on this!
Bill

[Alban (albeu)]

QSet vs. Qlist: The reading in order is pretty important IMHO. You want to read the chunks in the order they are requested, as it should be the order in which they'll be needed. Additionally if the order is kept it (may) avoid some seeking, which is an inherently slow operation. On the other hand preventing duplicate is not very important IMHO. If a block has just been read it will be in the cache and not read again. AFAICT the length of the list currently depend only on what the RAMAN request.

For the RAMAN I did try some other values but nothing larger would give me good results. However keep in mind that the CachingReader is still blocking the playing thread while it cache a block. Hence the more block you cache the more chances you have to hit a bad timing between the caching and playing thread. But I'm convinced that with a better designed CachingReader it would be possible to use much larger values without problems.

But beside all this there is a more fundamental problem. The seeking simply has to be fast enough, otherwise it might block the player which in turn block the main output. It would perhaps be better to allow the player to wait until the seek is done and enough data is cached before it continue playing. Or feed it with silence until the data is available.

[RJ Skerry-Ryan (rryan)]

Hey Alban,

Thanks a lot for your work on fixing this. I think you identified a problem area -- the lookahead of 64 callbacks is ridiculous and should be much shorter!

But I think the root of the problem is that we allow reads within the callback at all. I've just re-written CachingReader so that all reads occur in the reader thread, and the cache lives in the callback. They communicate via a thread-safe FIFO, so there is no need for locks at all.

I've pushed my changes to this branch: lp:~mixxxdevelopers/mixxx/cachingreader-fixes
Are you able to build from source? If so could you test this branch and verify that it fixes the issue for you on your netbook? If you aren't able to build from source then I could make a custom build for you if you let me know your OS.

Thanks!
RJ Ryan

[Alban (albeu)]

Great! I'll test that during the weekend.

[Owen Williams (ywwg)]

I think lp:~mixxxdevelopers/mixxx/cachingreader-fixes is missing util/pa_memorybarrier.h?

[RJ Skerry-Ryan (rryan)]

Oops, yes it is. added now

On Thu, Sep 29, 2011 at 8:00 AM, Owen Williams <email address hidden> wrote:

> I think lp:~mixxxdevelopers/mixxx/cachingreader-fixes is missing
> util/pa_memorybarrier.h?
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/840290
>
> Title:
> CachingReader block the player thread while it is caching
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/840290/+subscriptions
>

[RJ Skerry-Ryan (rryan)]

cachingreader-fixes is merged to trunk now and I believe this is fixed. All CachingReader methods called from the callback do not have any locks except for QWaitCondition::wakeAll() which we really can't do anything about short of forking QWaitCondition with our own version that does not use an internal mutex.

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/5980