On Fri, 26 Jun 2009 11:21:31 +0200 (CEST)
Nicolas Pennequin <nicolas.pennequin_at_free.fr> wrote:

> ----- "Rafaël Carré" <rafael.carre_at_gmail.com> wrote :
> > 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.

In fact filling is set to STATE_FULL, so
buffering_low_buffer_callback() asks for a refill, and
audio_fill_file_buffer will set filling to STATE_FILLING. After that
I'm not sure of what happens..

> > 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.

Ok

> > 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.

In fact it's less rare for targets with a small buffer (I have 2 of
them), so perhaps this corner case is not well handled.

> > This code is a bit complex for me, and I hope someone could enlight
> > me.
> >
> > 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 :)
>
> Nicolas

I'll continue my reading then, thanks for the pointers !

-- 
Rafaël Carré