Still a bug in with gstreamer0.10, Rhythmbox 0.9.5.

It would be relatively easy to do, just make the "quality" column check the mime-type. However there a are a few things that need considering:

* we'd then probably need to allow people to use "quality = lossless" for playlist criteria
* we'd need to make it sort correctly
* the list of what we consider lossless would need to be hardcoded, unless someone can think of a way to determine it at runtime

I think an "easy" way to do take care of number one (and maybe three) would have it to _display_ lossless, but actually hold the value 1411kbps or something along those lines.  

Created attachment 131750 [details] [review]
Patch File To Check for FLAC mime-type

This is a patch, which makes the info-widget check the mime-type for a flac file.

Your patch is backwards.  If you're working from a svn checkout, you can just do 'svn diff' to generate a patch to attach.

The comparison could be written a bit more simply:

  const char *type = rhythmdb_entry_get_string (entry, RHYTHMDB_PROP_MIMETYPE);
  if (g_str_equal (type, "audio/x-flac")) {
     ...
  } else {
     ...
  }

This only addresses one aspect of this problem (the song info dialog).  I'd suggest adding a helper function (probably in lib/rb-util.c) to figure out if a database entry represents a lossless encoding or not.  Initially this can just check if the media type is audio/x-flac, but it might get more complicated than that.

The other two things you'd need to do: modify the 'quality' cell data function in rb-entry-view.c to display _("Lossless") for lossless encodings, and add a specialised sort function for the quality column that sorts lossless entries appropriately.

I don't know what to do about allowing 'quality = lossless' in auto playlists.

(unless you're going to follow all rhythmbox bugzilla activity, you should add yourself to the cc list for bugs you're interested in)

What about changing how it imports the files? So that if it imports a flac file, it stores the bitrate as 1411.  It wouldn't fix the display for already imported files, but would technically solve the issue.

I don't like that idea at all.

Created attachment 131815 [details] [review]
Proposed patch to display FLAC files as Lossless

 - Added rb_check_lossless function to lib/rb-util.h and lib/rb-util.c
 - Modified rb-entry-view.c and rb-song-info.c to use rb_check_lossless and display accordingly.
 - rb-song-info.c now depends on rb-util.h

Not sure how to change the sorting order.  Does it use gtk_tree_view_column_set_sort_order to resort the entries when the sort order is changed?

Thanks for putting up with me.  Sorry if some of my questions/suggestions are sort of dumb, this is my first time contributing to a project.

>  gboolean
> +rb_check_lossless (const char *mime_type) {

I was thinking this would take a RhythmDBEntry, not a media type, since there may be other checks we'd want to do, such as checking if the URI starts with 'cdda://'.  'rb_entry_is_lossless' is probably a better name for it too.  Also, just as a style thing, the opening brace for a function body goes on the next line.

For sorting, the entry view widget and the source containing it take care of re-sorting the track list when the sort column changes.  Each column has an associated sort function that compares two entries.

All you need to do is add a new sort function to rhythmdb-query-model.c that compares the bitrate property taking lossless encodings into consideration.  You can base this on rhythmdb_query_model_ulong_sort_func, which is what is currently used for to compare bitrates.  Then, in widgets/rb-entry-view.c, use the new sort function for the 'quality' column (line 1488).

You should also add the translation for "Lossless" to the set of strings used to calculate the width of the quality column (lines 1491 to 1493 of rb-entry-view.c).

> Thanks for putting up with me.  Sorry if some of my questions/suggestions are
> sort of dumb, this is my first time contributing to a project.

Don't worry, you're doing fine.

I should probably explain why I don't like the idea of setting a fake bitrate.  It mostly just feels wrong to insert fake data to get sorting right when we don't really have enough information to do it correctly, and we can get the same results without it.

(In reply to comment #9)
> >  gboolean
> > +rb_check_lossless (const char *mime_type) {
> 
> I was thinking this would take a RhythmDBEntry, not a media type, since there
> may be other checks we'd want to do, such as checking if the URI starts with
> 'cdda://'.  'rb_entry_is_lossless' is probably a better name for it too.

I had made it a string because RhythmDBEntry is not defined in rb-util.h.  Should I include 'rhythmdb.h' into 'rb-util.h' (seems ugly), or how should I handle that?

Good point.  I guess that function belongs in rhythmdb.c.

Created attachment 131893 [details] [review]
Proposed patch to display FLAC files as Lossless

Implemented
-> rb_entry_is_lossless (RhythmDBEntry *entry) in rhythmdb.c
-> rhythmdb_query_model_bitrate_sort_func in rhythmdb-query-model.c
-> Set rb-entry-view.c to display "Lossless"
-> Set rb-song-info.c to display "Lossless"
-> Set rb-entry-view.c to sort using rhythmdb_query_model_bitrate_sort_func

Not Done
-> Add _("Lossless") to strings[] in rb-entry-view.c
        -> Reason: Causes SegFault?

If it's going in rhythmdb.c, it should be named rhythmdb_entry_is_lossless.  We sort of pretend rhythmdb is a different code base.  I don't really know why.

> Not Done
> -> Add _("Lossless") to strings[] in rb-entry-view.c
>         -> Reason: Causes SegFault?

The array currently only has four elements, and it needs to have a trailing NULL.  We just need to expand it to 5.

Other than that, this looks pretty good.

Created attachment 131945 [details] [review]
Proposed Patch

Fixed size of 'strings[]'

We could probably make the media type check more efficient.  Since it's a reference counted string, we can just do a pointer comparison.  That'd be a bit ugly though.

I was just about to commit this when I realised there's a bit of inconsistency here.  The sort function doesn't check that the bitrate value is 0 first, but the other two places do.  I don't think this would actually cause any problems, but the checks should all work the same way.

Created attachment 132141 [details] [review]
Proposed Patch

Unfortunately, if we have
if (a_val != b_val)
  do stuff
else
  lossless checks here

Since, Lossless files still technically have a bitrate of 0.
It will sort the files like: 320kbps, 120kbps, Lossless, Unknown

So, we have to run the lossless tests first to sort them: Lossless, 320kbps, 120kbps, Unknown

am I correct on that?

(regardless, I cleaned up the sorting function, into two if-else blocks)

> Since, Lossless files still technically have a bitrate of 0.
> It will sort the files like: 320kbps, 120kbps, Lossless, Unknown
> 
> So, we have to run the lossless tests first to sort them: Lossless, 320kbps,
> 120kbps, Unknown
> 
> am I correct on that?
> 

Yes, that's correct.  I didn't really think it through.  It's not really important, certainly less important than having the code actually work properly.

(In reply to comment #16)
> Created an attachment (id=132141) [edit]
> Proposed Patch
> 
> Unfortunately, if we have
> if (a_val != b_val)
>   do stuff
> else
>   lossless checks here
> 
> (regardless, I cleaned up the sorting function, into two if-else blocks)

I much preferred the previous version.  The ternary operator almost always makes the code harder to read, without really buying much.

I've tidied up a few things, tested a bit to make sure I didn't break it too obviously, and committed it.  We can use a new bug for the playlist criteria thing if you're interested in working on that.

2009-04-07  Jonathan Matthew  <jonathan@d14n.org>

        patch by:  Paul Bellamy  <paul.a.bellamy@gmail.com>

        * rhythmdb/rhythmdb.c: (rhythmdb_entry_is_lossless):
        * rhythmdb/rhythmdb.h:
        Add a function to determine if an entry represents a lossless encoded
        stream.  This currently just checks that the bitrate field is 0 and
        the media type is 'audio/x-flac'.  More media types and other checks
        can be added later.

        * rhythmdb/rhythmdb-query-model.c:
        (rhythmdb_query_model_bitrate_sort_func):
        * rhythmdb/rhythmdb-query-model.h:
        Add a query model sort function for bitrate comparisons, sorting
        lossless encodings as being of higher quality than anything with a
        bitrate.  Not exactly correct, but it's the best we can do.

        * widgets/rb-entry-view.c: (rb_entry_view_quality_cell_data_func),
        (rb_entry_view_append_column):
        * widgets/rb-song-info.c: (rb_song_info_update_bitrate):
        Display "Lossless" for lossless encodings in the 'quality' column in
        track lists and the same field in the song info dialog.

        Fixes #337438.

Sure, that sounds good.

With rhythmbox 0.12.2, many of my flac files are now "Lossless", but some are still "Unknown". 

They're probably flac-in-ogg or flac files with id3 tags.  We can't display those properly without some significant changes elsewhere.