----- "Rafaël Carré" <rafael.carre_at_gmail.com> wrote :
> I'm reading playback.c and buffering.c to find what is wrong with the
> Clip/c200v2/m200v4 (targets with quite small audiobuffer).
> I have already identified a problem related to album art (on the
> color c200v2):
> audio_finish_load_track() in playback.c returns early because album
> art couldn't be loaded (bofopen() returns ERR_BUFFER_FULL).
> This causes an infinite loop : buffering_thread() keeps sending a
> BUFFER_LOW event and playback.c's buffering_low_buffer_callback()
> will have no effect since filling is STATE_FILLING.
I can't see where this is the case. In the #ifdef HAVE_ALBUMART block that is in audio_finish_load_track(), if bufopen() returns ERR_BUFFER_FULL, filling is set to STATE_FULL and thus buffering should stop. This looks like what you mention later, so I must be failing to see the case you're talking about.
> If AA couldn't be loaded because of another error, the function
> continues loading the codec. This behaviour was introduced by Toni in
> r20149 "Bugfix: If AlbumArt bitmap loading fails, dont try loading it
> over and over again, but simply ignore AlbumArt in this case."
> Repeating the change from r20149 "works for me" in the sense that the
> song starts playing without album art.
> However I feel like this change was not correct in the first place:
> audio_finish_load_track() will return in several other cases,
> commented as "buffer is full, not an error".
> Some of these comments have been committed by kugel in r20093, which
> also handled specifically cases where the buffer is full.
> If it's not an error I assume this function will be called again when
> some buffer will have been consumed by playing back the current song
> but this doesn't seem to be the case : audio_finish_load_track() is
> only called on Q_AUDIO_FINISH_LOAD event, itself only called on
> BUFFER_EVENT_FINISHED event (which means the current song has been
> fully buffered?)
Your assumption is correct. Once the buffer empties, the track loading will be attempted again (IIRC from scratch (meaning the previously loaded metadata is not reused), but I'm a bit fuzzy on the details, since it's been some time since I've worked on that code).
audio_finish_load_track() is called after the metadata for a track has been loaded. It is this way because metadata is loaded on the buffering thread, so once this is done the audio thread is notified and carries on with the rest of the data for the track.
> There is something wrong there but I just can't figure what:
> - Is rockbox meant to infinitely (or perhaps in a clever manner)
> retry loading a track until there is enough buffer available?
> - If the buffer is full when attempting to load album art or a codec
> handle, is that an unrecoverable error?
The buffer reaching the full state while attempting to load something other than audio data is a perfectly normal (although relatively rare) condition. The filling should simply stop until another one occurs, where the data for that particular track should fit in the buffer, since it will now be the first one to be loaded.
> This code is a bit complex for me, and I hope someone could enlight
> Perhaps playback.c is something no one understands anymore? (644
> revisions :/)
Unfortunately, it has always suffered from the fact that few people understand it. I like to believe that this has gotten better since playback and buffering were separated, but the situation is admittedly still not ideal. I can only encourage you to continue trying to get familiar with it, since more eyes on a piece of code are always a good thing :)
Received on 2009-06-26