GNOME Bugzilla – Bug 608995
[recorder] Make parameters configureable
Last modified: 2010-03-31 11:45:15 UTC
Make the framerate, filename and gstreamer pipeline used by the screencast recorder configureable using gconf. This patch does not change the defaults, it justs provides a way for the user to override them.
Created attachment 153015 [details] [review] [recorder] Make parameters configureable
Created attachment 153032 [details] [review] [recorder] Make parameters configureable Use correct CFLAGS
Review of attachment 153032 [details] [review]: Hmm, thinking about this some having Shell recorder read GConf directly really doesn't fit in with the fact that the parameters that you are setting via GConf are configurable parameters of the ShellRecorder object. (Well, some of them - the FPS isn't currently settable, but could be made so) It would make more sense to me if the GConf handling was done in the JS code, and that configured the recorder object. (The idea I had was that the recorder object is meant to be reusable code that could be pulled out of the shell if people were interested - not sure that's ever going to happen but it's the "concept" of the way things are set up, so I think it's best to stick to that. Sorry for not thinking of that when we were discussing shell-gconf usage before. Other comments: - The details in the API docs on the pipeline and filename should really be in the GConf key descriptions as well (but see my next comment about the filename) - Maybe we should only allow setting directory and extension for the filename? (as two separate keys) I'm not sure that the full pattern is that interesting to configure statically in GConf. - I'd probably make the key for the pipeline have an empty default value, and just mention what is used if empty or unset in the GConf key description. That would allow natural evolution if we later want to have high-level keys like "Format for recordings" or "Downscale recordings to <WxH> before encoding"; then the explicit pipeline would just override all of that if set to something non-empty.
(In reply to comment #3) > Review of attachment 153032 [details] [review]: > > Hmm, thinking about this some having Shell recorder read GConf directly really > doesn't fit in with the fact that the parameters that you are setting via GConf > are configurable parameters of the ShellRecorder object. (Well, some of them - > the FPS isn't currently settable, but could be made so) It would make more > sense to me if the GConf handling was done in the JS code, and that configured > the recorder object. (The idea I had was that the recorder object is meant to > be reusable code that could be pulled out of the shell if people were > interested - not sure that's ever going to happen but it's the "concept" of the > way things are set up, so I think it's best to stick to that. Yeah that would be indeed cleaner. > Sorry for not thinking of that when we were discussing shell-gconf usage > before. > > Other comments: > > - The details in the API docs on the pipeline and filename should really be in > the GConf key descriptions as well (but see my next comment about the filename) Well I did not really think much about the descriptions, they indeed can be improved. > - Maybe we should only allow setting directory and extension for the filename? Yeah the extension was the reason why I made it optional at all (when you use a different muxer like mp4, avi or mpeg you don't want an ogg extension). As for the directory, yeah could change it to, it would make more sense to use ~/Videos as default instead of the home directory. > (as two separate keys) I'm not sure that the full pattern is that interesting > to configure statically in GConf. It isn't really because it does not matter much (it just makes sure that the filename is unique, you'd want to rename it latter anyway). > - I'd probably make the key for the pipeline have an empty default value, and > just mention what is used if empty or unset in the GConf key description. That > would allow natural evolution if we later want to have high-level keys like > "Format for recordings" or "Downscale recordings to <WxH> before encoding"; > then the explicit pipeline would just override all of that if set to something > non-empty. OK, that sounds like a good idea.
Created attachment 153131 [details] [review] [recorder] Make parameters configureable Changes: *) Set gconf values from javascript rather than from within shell-recorder.c *) Add a framerate property *) Make the file extension configurable rather than the filename pattern *) Default to an empty string for the pipeline (the default one is if empty) Still have not done anything to the directory, I am not sure whether we want to make it configurable or just default to ~/Video
Created attachment 153133 [details] [review] [recorder] Make parameters configureable Some cleanups
(In reply to comment #5) > Still have not done anything to the directory, I am not sure whether we want to > make it configurable or just default to ~/Video I'm not sure about an option either, but I'd hate a default of ~/Video - after all, that's what xdg-user-dirs are there for, right?
(In reply to comment #7) > (In reply to comment #5) > > Still have not done anything to the directory, I am not sure whether we want to > > make it configurable or just default to ~/Video > > I'm not sure about an option either, but I'd hate a default of ~/Video - after > all, that's what xdg-user-dirs are there for, right? I should be more clear about this ;) By "~/Video" (should be ~/Videos) I meant the xdg-user-dir used for Videos (~/Videos happens to be the default; that's why I used it in my comment).
Created attachment 153135 [details] [review] [recorder] Make parameters configureable More small fixes / cleanups.
Review of attachment 153135 [details] [review]: Looking good, a few comments in detail below. Commit message should describing adding the framerate property to ShellRecorder In the commit message fileextension => file extensions ::: data/gnome-shell.schemas @@ +141,3 @@ + <short>The gstreamer pipeline used to encode the screencast</short> + <long> + Sets the GStreamer pipeline used to encode recordings. It follows the syntax used for gst-launch. The pipeline should have an unconnected sink pad where the recorded video is recorded. It will normally have a unconnected source pad; output from that pad will be written into the output file. However the pipeline can also take care of its own output - this might be used to send the output to an icecast server via shout2send or similar. Please line-wrap these to a reasonable width. You should also include a few words about what happens when it is set to an empty value. Something like: "When unset or set to an empty value, the default pipeline will be used. This is currently '... ! ... ! ...' and records to Ogg Theora." @@ +155,3 @@ + <short>File extension used for storing the screencast</short> + <long> + GNOME Shell's screencast recorder uses this file extension to store the screencasts in the user's home directory. It should be changed when recording to a different container format. I'd be a bit more expansive. Maybe replace the first sentence with something like "The filename for recorded screencasts will be a unique filename based on the current date, and use this extension". ::: js/ui/main.js @@ +132,2 @@ recorder = new Shell.Recorder({ stage: global.stage }); + recorder.set_framerate(gconf.get_int("recorder/framerate")); I think you should hook up change notification - people shouldn't have to restart the shell to fool around with changing the screencast parameters. @@ +133,3 @@ + recorder.set_framerate(gconf.get_int("recorder/framerate")); + recorder.set_filename("shell-%d%u-%c." + gconf.get_string("recorder/file_extension")); + if (gconf.get_string("recorder/pipeline") != "") { It's hard on the user if ' ' is different than ''. I'd probably use val.match(/^\s*$/) to check for all-white. ::: src/shell-recorder.c @@ +252,2 @@ recorder->state = RECORDER_STATE_CLOSED; + recorder->framerate = FRAMES_PER_SECOND; This should be renamed to DEFAULT_FRAMES_PER_SECOND @@ +1009,3 @@ + g_param_spec_int ("framerate", + "Framerate", + "Framerate used for resulting video", , in frames-per-second @@ +1564,3 @@ + * shell_recorder_set_framerate: + * @recorder: the #ShellRecorder + * @filename: Framerate used for resulting video. @framerate, not @filename , in frames-per-second @@ +1566,3 @@ + * @filename: Framerate used for resulting video. + * + * Sets the number of frames per second we configure for the GStreamer pipeline. I'd probably not reference @@ +1578,3 @@ + recorder_set_framerate (recorder, framerate); + +} Extra blank line
(In reply to comment #10) > Review of attachment 153135 [details] [review]: > > Looking good, a few comments in detail below. > > Commit message should describing adding the framerate property to ShellRecorder > In the commit message fileextension => file extensions OK > ::: data/gnome-shell.schemas > @@ +141,3 @@ > + <short>The gstreamer pipeline used to encode the screencast</short> > + <long> > + Sets the GStreamer pipeline used to encode recordings. It follows > the syntax used for gst-launch. The pipeline should have an unconnected sink > pad where the recorded video is recorded. It will normally have a unconnected > source pad; output from that pad will be written into the output file. However > the pipeline can also take care of its own output - this might be used to send > the output to an icecast server via shout2send or similar. > > Please line-wrap these to a reasonable width. You should also include a few > words about what happens when it is set to an empty value. Something like: > "When unset or set to an empty value, the default pipeline will be used. This > is currently '... ! ... ! ...' and records to Ogg Theora." OK > @@ +155,3 @@ > + <short>File extension used for storing the screencast</short> > + <long> > + GNOME Shell's screencast recorder uses this file extension to > store the screencasts in the user's home directory. It should be changed when > recording to a different container format. > > I'd be a bit more expansive. Maybe replace the first sentence with something > like "The filename for recorded screencasts will be a unique filename based on > the current date, and use this extension". OK > ::: js/ui/main.js > @@ +132,2 @@ > recorder = new Shell.Recorder({ stage: global.stage }); > + recorder.set_framerate(gconf.get_int("recorder/framerate")); > > I think you should hook up change notification - people shouldn't have to > restart the shell to fool around with changing the screencast parameters. They don't have to, the object is destroyed once recording is done and recreated every time recording starts. So you start to record, change a setting it picks it up the next time you record, no restart required. > @@ +133,3 @@ > + recorder.set_framerate(gconf.get_int("recorder/framerate")); > + recorder.set_filename("shell-%d%u-%c." + > gconf.get_string("recorder/file_extension")); > + if (gconf.get_string("recorder/pipeline") != "") { > > It's hard on the user if ' ' is different than ''. I'd probably use > val.match(/^\s*$/) to check for all-white. Yeah that makes sense. > ::: src/shell-recorder.c > @@ +252,2 @@ > recorder->state = RECORDER_STATE_CLOSED; > + recorder->framerate = FRAMES_PER_SECOND; > > This should be renamed to DEFAULT_FRAMES_PER_SECOND OK > @@ +1009,3 @@ > + g_param_spec_int ("framerate", > + "Framerate", > + "Framerate used for > resulting video", > > , in frames-per-second OK > @@ +1564,3 @@ > + * shell_recorder_set_framerate: > + * @recorder: the #ShellRecorder > + * @filename: Framerate used for resulting video. > > @framerate, not @filename > > , in frames-per-second heh ... copy & paste slipped ;) > @@ +1566,3 @@ > + * @filename: Framerate used for resulting video. > + * > + * Sets the number of frames per second we configure for the GStreamer > pipeline. > > I'd probably not reference You mean using something like "Sets the number of frames per second used for the GStreamer pipeline." ? > @@ +1578,3 @@ > + recorder_set_framerate (recorder, framerate); > + > +} > > Extra blank line OK, will remove it. Thanks for the review.
(In reply to comment #11) > > ::: js/ui/main.js > > @@ +132,2 @@ > > recorder = new Shell.Recorder({ stage: global.stage }); > > + recorder.set_framerate(gconf.get_int("recorder/framerate")); > > > > I think you should hook up change notification - people shouldn't have to > > restart the shell to fool around with changing the screencast parameters. > > They don't have to, the object is destroyed once recording is done and > recreated every time recording starts. > > So you start to record, change a setting it picks it up the next time you > record, no restart required. Really? I don't see that in the code. It would be possible to move the set-the-parameters-from-gconf code outside the if (recorder == null) block instead of adding change notification. I'd be fine with that approach, though it probably needs a comment like //read the parameters from GConf always in case they have changed > > @@ +1566,3 @@ > > + * @filename: Framerate used for resulting video. > > + * > > + * Sets the number of frames per second we configure for the GStreamer > > pipeline. > > > > I'd probably not reference > > You mean using something like "Sets the number of frames per second used for > the GStreamer pipeline." ? Left-over stub of a comment I decided against. (I was going to say that maybe we shouldn't reference GStreamer in this comment since the framerate would make sense even if we weren't using GStreamer, but then decided that we have GStreamer locked into the API anways, and what you had was fine>
(In reply to comment #12) > (In reply to comment #11) > > > > ::: js/ui/main.js > > > @@ +132,2 @@ > > > recorder = new Shell.Recorder({ stage: global.stage }); > > > + recorder.set_framerate(gconf.get_int("recorder/framerate")); > > > > > > I think you should hook up change notification - people shouldn't have to > > > restart the shell to fool around with changing the screencast parameters. > > > > They don't have to, the object is destroyed once recording is done and > > recreated every time recording starts. > > > > So you start to record, change a setting it picks it up the next time you > > record, no restart required. > > Really? I don't see that in the code. It wasn't in the original code, but it is in the patch: if (recorder.is_recording()) { recorder.pause(); + recorder = null; I added this exactly for this reason. > It would be possible to move the > set-the-parameters-from-gconf code outside the if (recorder == null) block > instead of adding change notification. I'd be fine with that approach, though > it probably needs a comment like > > //read the parameters from GConf always in case they have changed Currently it is in the if block because this is called every time you start recording (initially recorder is null, and after recording it is null too), but not when you stop recording (recording != null). Putting the reading outside of the if block is possible but reading the gconf entries when recording stops isn't really necessary. > > > @@ +1566,3 @@ > > > + * @filename: Framerate used for resulting video. > > > + * > > > + * Sets the number of frames per second we configure for the GStreamer > > > pipeline. > > > > > > I'd probably not reference > > > > You mean using something like "Sets the number of frames per second used for > > the GStreamer pipeline." ? > > Left-over stub of a comment I decided against. (I was going to say that maybe > we shouldn't reference GStreamer in this comment since the framerate would make > sense even if we weren't using GStreamer, but then decided that we have > GStreamer locked into the API anways, and what you had was fine> OK
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > > > > ::: js/ui/main.js > > > > @@ +132,2 @@ > > > > recorder = new Shell.Recorder({ stage: global.stage }); > > > > + recorder.set_framerate(gconf.get_int("recorder/framerate")); > > > > > > > > I think you should hook up change notification - people shouldn't have to > > > > restart the shell to fool around with changing the screencast parameters. > > > > > > They don't have to, the object is destroyed once recording is done and > > > recreated every time recording starts. > > > > > > So you start to record, change a setting it picks it up the next time you > > > record, no restart required. > > > > Really? I don't see that in the code. > > It wasn't in the original code, but it is in the patch: > > > if (recorder.is_recording()) { > recorder.pause(); > + recorder = null; > > I added this exactly for this reason. Ah, missed that. - I don't really like the way it fits in with the code (would at least require broader changes in the JS code to make the code make sense), - If you start a new recording while the old recording is still going, you'll get overlapping indicators. - It also breaks the way that the recorder numbers recordings, with the distinction between multiple segments of the same recording and separate recordings between shell restarts. I'm not sure that the distinction is *useful* but again, if we wanted to remove the funky recording numbering then we should remove it, and not leave it there and not leave it there but non-working. And that would be a separate change. I think it's easiest here to just leave the recorder object around and reset the parameters on restart. The ShellRecorder code is careful to short-circuit all setting of parameters back to the same values.
Created attachment 153286 [details] [review] [recorder] Make parameters configureable *) Fixed up patch. *) Leave the recorder object around and reset the parameters on restart
Review of attachment 153286 [details] [review]: One simple code problem and a few whitespace issues noted below. Otherwise, looks great! feel free to commit with the fixes below. ::: js/ui/main.js @@ +140,3 @@ + recorder.set_filename("shell-%d%u-%c." + gconf.get_string("recorder/file_extension")); + if (!gconf.get_string("recorder/pipeline").match(/^\s*$/)) { + recorder.set_pipeline(gconf.get_string("recorder/pipeline")); This should be let pipeline = gconf.get_string("recorder/pipeline"); if (!pipeline.match(/^\s*$/)) recorder.set_pipeline(pipeline); else recorder.set_pipeline(null); to deal with the case of setting the GConf key back to an empty value. ::: src/shell-recorder.c @@ +881,3 @@ static void +recorder_set_framerate (ShellRecorder *recorder, + int framerate) Parameters need to be aligned in columns @@ +1572,3 @@ +void +shell_recorder_set_framerate (ShellRecorder *recorder, + int framerate) Parameters need to be aligned in columns. Looks like 'int framerate' is one column too far to the left - it should line up with ShellRecorder. ::: src/shell-recorder.h @@ +32,3 @@ +void shell_recorder_set_framerate (ShellRecorder *recorder, + int framerate); Parameters need to be lined up in columns.
Comment on attachment 153286 [details] [review] [recorder] Make parameters configureable Commited with the fixes from Comment 16 as 3e1b1d5
*** Bug 576132 has been marked as a duplicate of this bug. ***