Re: [Rhythmbox-devel] playlist source burner 0.3

[William Jon McCann <mccannwj pha jhu edu>]

Hi Christophe,

Christophe Fergeau wrote:
> Ok, so now it's my turn to nitpick. I feel a bit guilty about bugging
> you for details given the amount of work you probably put in that
> patch ;)

Good comments are really appreciated.

> +AM_CONDITIONAL(MKDTEMP_MISSING, test x$mkdtemp_missing = xtrue)
> Won't compilation fail on systems without mkdtemp ?

I think I copied this right out of nautilus-cd-burner.  The check is 
used to pull in a header file in case the function is not defined on the 
system.  Is this function provided in libnautilus-burn if not by the system?

> +	/*g_object_set (widget, "show-recorders-only", TRUE, NULL);*/
>
> Why  is that commented out?

It depends on a change to bacon-cd-selector that I just sent to Bastien.

> +burn_playlist_iter_func (GtkTreeModel *model, GtkTreeIter *iter, char
> **uri, char **artist, char **title, int *duration)
>
> entry->duration is a gulong, probably better to use a gulong here too

OK.

> +void
> +rb_playlist_source_burn_playlist (RBPlaylistSource *source)
>
>
> +	g_object_get (source, "name", &name, NULL);
>
> name is never freed

OK.

> +	if (error)
> +		g_error_free (error);
>
> Looks a bit weird to pass a GError to the previous function if it's not
> used, NULL would probably be enough. I guess this code means "error
> handling is planned, but not implemented yet" ?

Correct.  I'll change it to just use NULL for now though.

> +static GObject*
> +rb_playlist_source_recorder_constructor (GType                  type,
>
> Any reason why you are using the object constructor instead of the
> object init function?

I don't recall.  I think I was looking at how the file-chooser worked or 
something.  So perhaps, no good reason.

> +                if (error) {
> +                        g_warning ("Invalid writer device: %s",
> device);
> +                        /* ignore and let rb_recorder try to find a
> default */
> +                }
>
> Should probably be marked for translation

Do we translate warnings sent to the console?

> +GtkWidget *
> +rb_playlist_source_recorder_new (GtkWidget *parent,
> +                                 char      *name)
>
> name is const char *

OK.

> +void
> +rb_playlist_source_recorder_add_from_model (RBPlaylistSourceRecorder 
>
> Any reason why you aren't using gtk_tree_model_get_iter_first and
> gtk_tree_model_iter_next in that function instead of
> gtk_tree_model_get_iter_from_string?

No good reason.  I've changed it now.

> +static long
> +rb_playlist_source_recorder_estimate_total_size
> (RBPlaylistSourceRecorder *source)
>
> If I'm not mistaken, this will overflow for about 200 minutes of music:
> 2^31 / AUDIO_RATE ~= 12000 secs = 200 minutes (don't remember if a long
> is signed or not). A guint64 is probably safer. It seems this function
> is only called after checking that the total length won't exceed the
> length of a CD, so it's probably safe, but maybe there can be "audio
> dvds" where this can be an issue?

OK.

> +static gboolean
> +check_dir_has_space (const char *path,
> +                     long        bytes_needed)
>
> I'd recommend implementing this function using 
> GnomeVFSResult
> gnome_vfs_get_volume_free_space (const GnomeVFSURI *vfs_uri, 
> 				 GnomeVFSFileSize  *size);
>
> See the #ifdef mess for portability in gnome-vfs/libgnomevfs/gnome-vfs-
> utils.c if you want an explanation ;) The various statfs/statvfs checks
> can probably go away too if you agree with using that function.

Great.  Much better.

> +                if ((media_duration < 0) && (duration > 4800)) {
>
> Fwiw, the CD could be a 74 min CD, and I think there are 90 and 99
> minutes CDs iirc. Maybe we should ask the user what to do when we
> couldn't read the media size and the length is greater than 74*60?

I'm fine with changing back to 74 minutes.  I suppose asking the user is 
ok but I hate to add more dialogs.  Maybe it is better to ask the user 
to insert the media and verify the size correctly?

> +        /* The main playback pipeline, at the end this looks like:
> +         *  { src ! spider ! audioconvert ! wavenc ! sink }
>
> I think the pipeline needs to be src ! spider ! audioconvert !
> audioscale ! audio/x-raw-int,rate=44100,width=16,depth=16 ! wavenc !
> sink
> Maybe spider will be able to do the right thing with only
> src ! spider ! audio/x-raw-int,rate=44100,width=16,depth=16 ! wavenc !
> sink

Not sure.  It seems to work as it is.

> +        dummy = gst_element_factory_make ("fakesink", "fakesink");
> +        if (!dummy
> +            || !gst_scheduler_factory_make (NULL, GST_ELEMENT (dummy)))
> {
> +                g_set_error (error, RB_RECORDER_ERROR,
> +                             RB_RECORDER_ERROR_GENERAL,
> +                             _("Couldn't initialize scheduler.  Did you
> run gst-register?"));
> +                return NULL;
> +        }
>
> This check is probably already done elsewhere in rhythmbox, there's no
> need to duplicate it here imo.

I think that is only true if you play audio before attempting to write a 
CD.  I think I'll keep it for now.

> +                if (may_pause == -1) {
> +                        guint major, minor, micro;
> +
> +                        rb_debug ("decoding gst version");
> +                        gst_version (&major, &minor, &micro);
> +                        /* this makes sure someone removes this later
> on */
> +                        g_assert (major == 0);
> +                        g_assert (minor == 8);
> +                        if (micro >= 1)
> +                                may_pause = 1;
> +                        else
> +                                may_pause = 0;
> +                }
>
> may_pause doesn't seem to be used apart from that piece of code, which
> is there to properly free the sound device when playback is paused, so I
> guess it can be removed.

OK.  I got this from the player object originally.  I figured it must 
have been there for a reason.  I think you are right so I've removed it now.

> +static char *
> +get_dest_from_uri (const char *tmp_dir,
>
> I don't really understand what this function does, to my eyes, it
> doesn't seem to guarantee much about the filename: it ends in .wav, it's
> in the temporary directory, but maybe a filename with the same name
> already exists, and nothing prevents this file from being removed or
> replaced with a link or something before we use it. Maybe I missed
> something though, I'm tired atm ;) If I'm right, it may be better to
> return the open fd from g_mkstemp (we don't care about the .wav
> extension, do we?) and use fdsink. This probably doesn't matter much
> anyway.

Mostly true.  We do need the files to have a .wav extension.  From the 
cdrecord man page:

"cdrecord recognizes that audio data in a .WAV file is stored in Intel 
(little-endian) byte order, and will automatically byte-swap the data if 
the CD recorder requires big-endian data."

It is extremely unlikely that a filename with the same name exists.  We 
create a temporary directory using mkdtemp that has a unique name.  We 
create unique files in that directory using g_mkstemp.  We create 
another file with a ".wav" attached to that name.

The g_mkstemp files are kept around until we are done so that we don't 
try to write to the same ".wav" file.  So, it is impossible for the same 
process of rhythmbox to clobber its own files.  It is highly unlikely 
that another process will create a ".wav" file with the same name 
between the time we create the g_mkstemp file and the time we start the 
pipeline running.  If it does, then worst case is that the pipeline fails.

On the other hand, I suppose we could use fdsink and then use gnome-vfs 
to safely rename to a .wav extension before burning.

> +static gint64
> +get_tracks_length (RBRecorder *recorder,
> +                   GError    **error)
>
> We should already know the track length because we converted the track
> to wav, maybe we don't need to reread it from the wav file? Actually,
> the checks being done in rb_recorder_burn have already been done
> previous in rb_playlist_source_recorder_start, didn't they?

Sorta.  We can't trust those durations.  This check was recommended by 
Bastien.  I think it is a good one.  It is really the only authoritative 
check we do.  Also, if this object is used for something else it is 
useful to do the check there.

> Ok, I'm done with all my annoying comments, I hope what I'm saying is
> making sense, feel free to disagree with some of them ;)

Thanks for the good feedback!

> As a side note, do you think your audio=>wav converter would be easy
> enough to reuse for an audio=>another audio format converter? That would
> be useful for transferring files to an iPod for example ;)

Interesting.  Maybe you could just generalize the output encoder, size 
check, and instead of a burn use a move.  I've tried to keep generality 
in mind for the recorder object.  For example, I've attempted to hide 
all the details of what type of media the destination is.  I just 
noticed that the only indication in the API is in the RBRecorderAction 
enum, where I use the term DISC.

> Thanks for this patch which looks great ;)
>
> Christophe