Created attachment 104533 [details] [review]
Proposed patch

Thanks for this!

We'd need to switch GStreamer element before this can be used, because otherwise there will be an interesting mismatch between what URLs the UI and the extractor are using.

Of course, once it's all GIO, the URLs can be dropped and replaced with GFiles.

Maybe I should create a gio branch for this...

Potentially, it should be using the gio cdda backend as well for input, so that concurrent access to mixed type discs works as expected.

To be honest I'd prefer to use gstreamer's CDDA source.  I've been using it locally for a few weeks now and it seems good.  A little slower than cdparanoia, but I've no idea at what level of "paranoia" it is using.

(In reply to comment #4)
> To be honest I'd prefer to use gstreamer's CDDA source.  I've been using it
> locally for a few weeks now and it seems good.  A little slower than
> cdparanoia, but I've no idea at what level of "paranoia" it is using.

You mean gstreamer's libcdio source I guess? That's what the gvfs cdda backend uses as well.

I've made a bzr branch of SJ, and committed that patch to it:

https://code.launchpad.net/~ross/sound-juicer/gio

Next up:
* Sanitise libjuicer API.  All file references should be via GFile, nothing
else.
* Switch to giosink

I don't have time right now to work on this, so if you want to continue working
on this Michael, that would be greatly appreciated!


Both the gstreamer cdda element and gvfs cdda element use libcdio, but the gstreamer element would integrate better surely.

Yeah, I'm interested in continuing the porting work.

Created attachment 104853 [details] [review]
New patch against bzr

OK, here's a further patch against that bzr tree.  It converts libjuicer to GFile (make_directory, get_default_music_dir, get_submit_url).  It also uses GFile more consistently in src/.

It's possible libjuicer could use GDrive instead of device character strings, but those strings come from nautilus and it seemed more effort than it was worth to convert from string->GDrive->string, since gstreamer wants the string too.

I switched to giosink (you need to compile gst-plugins-base with --enable-experimental to get it).

I left cdda alone for now, as it seemed tangental to gio.

Mostly committed and pushed, thanks!

You shouldn't change the URLs returned by the metadata backends, those really are plain URL strings.  I've reverted these changes.

Instead of having sj_get_default_music_directory in sj-utils, we can now use g_get_user_special_dir() directly.

Created attachment 104871 [details] [review]
Use g_get_special_dir

I'd argue that since g_get_special_dir can return NULL, sj_get_default_music_directory is just as useful as before (to automatically provide the fallback value of ~).

But g_get_special_dir does mean that we can drop xdg_user_dir_lookup.  Here's a patch to do that.

"The GUserDirectory enumeration can be extended at later date. Not every platform has a directory for every logical id in this enumeration."

This means that (probably) Windows won't be able to find some special directories. That's the only case for which you'd get a NULL back from g_get_user_special_dir(). But you don't have any assurance that the directory will exist.

All in all, the semantics are different, so it's not a straight replacement.

NULL doesn't appear to be that odd a case.  On Linux, if user-dirs.dirs doesn't exist, isn't readable, or doesn't have the right entry, g_get_user_special_dir will return NULL.  Certainly the API doesn't guarantee anything.  I don't know how common those cases are, but they seem worth coding around.

What are the differing semantics?  xdg_user_dir_lookup and g_get_user_special_dir look pretty similar to me.  Neither of them assures that the directory exists.

Committed, thanks.

so the port to gvfs is not yet resolved only because of gnomevfssink, if i got it right?

Yes.  When giosink is in gst-plugins-good, I'll commit this.  If the gstreamer people commit to doing that "shortly" I'll put this into sj trunk though.

(In reply to comment #15)
> Yes.  When giosink is in gst-plugins-good, I'll commit this.  If the gstreamer
> people commit to doing that "shortly" I'll put this into sj trunk though.

It's already in -base, when --enable-experimental is enabled for configure.

And its packaged in Debian too... Time to branch!

I'll happily test and merge the GIO patch, but first it needs updating for the changes in svn.

https://code.launchpad.net/~ross/sound-juicer/gio is the bzr branch with the code in, a simple "bzr merge" will pull in master and cause a few conflicts which need to be fixed.

OK, assuming I did everything correctly, you should find merged code: https://code.launchpad.net/~mterry/sound-juicer/gio

The conflict arose because s-j trunk had added support for first creating files at .FILENAME and then moving them to FILENAME when encoding completed.  If s-j can assume that files will be written using gio, it may not need to do that (I believe gio promises atomic replacements).  But, it seems safe enough to leave it in, so I did.

Thanks, I'll merge this shortly.  This isn't for atomic operations, but so that if you start ripping, overrwiting an existing file and then cancel, you don't lose the original file.

Merged, thanks.

One last thing... (I sound a little like Columbo) there are several g_file_* calls in sj-extracting which can fail but are not checked for (specifically g_file_move and g_file_delete).  Please add a GError argument and handle errors corectly.  Then I think we're good for merging!

When I said atomic, I meant "doesn't overwrite unless operation is successfully completed".  i.e. if you cancelled it, gio shouldn't mess with original file.  At least that's what I gather from the docs for g_file_replace:

"This will try to replace the file in the safest way possible so that any errors during the writing will not affect an already existing copy of the file. For instance, for local files it may write to a temporary file and then atomically rename over the destination when the stream is closed."

As for GError's, you're right that they should be there, and I'll add them, but in my defense I'll note that the original code didn't handle errors there either!

OK, merge from my bzr branch again.  I've merged with HEAD and added GError support to the delete/move calls.  The delete call uses the generic "Sound Juicer could not extract this CD" error, which is a little odd since you're cancelling the extraction, but I left it alone.  Wasn't sure "could not clean up after itself" was any better.

Regarding my 'atomic' statements about giosink.  I've filed bug 529300 to allow giosink to replace files.  SJ wants this anyway, because otherwise if the temporary file already exists, we can't replace it.  So, my branch really recommends that that patch be applied (having an already-there temporary file is unusually, but probably not impossible).

Once the 529300 patch is applied and giosink uses atomic writes, SJ still can't just drop support for using the create_temporary/move logic.  SJ wants to delete the incomplete file if the user hits Cancel.  But when SJ cancels the gst operation, giosink just closes the stream and replaces the original file with the incomplete one.  Which isn't what we want.  So, in summation, I retract my claim that giosink can solve the overwrite problem for us.

Are you sure the overwrite signal is required?  There doesn't appear to be such a signal in giosink.

You're right, the signal isn't in giosink.  I submitted a patch in bug 529300 to add it, based on the previous gnomevfssink signal.  But as you can see in that bug, exactly how to allow overwriting is in question.

SJ is interested in this because:

Say, you want to extract the file "FOO".  Normally, SJ tells giosink to write ".FOO" and then moves it to the correct place when the write is complete.

But imagine ".FOO" already exists.  giosink will report an error because it never overwrites anything (in HEAD, it returns an odd error about not being able to *read* -- this is a typo in giosink).

Now, either giosink could gain the ability to overwrite (see abovementioned bug 529300) or SJ could solve this by first deleting any existing ".FOO" before writing.  After all, we'd be pretty sure it's our own trash.

But for now, I figured I'd try to get it into giosink proper, since it seems like a generally useful ability (to overwrite the sink target).

My bzr branch will be updated depending on the outcome of where to put this functionality, and I'll comment here.

Could we please get the code in sound-juicer trunk? Then we can work on finishing whatever needs to be done with the giosink.

Committed to trunk!