GNOME Bugzilla – Bug 632595
Recorder: Switch to webm
Last modified: 2011-02-10 14:36:04 UTC
Currently we default to ogg theora as encoding format as it was pretty much only our choice, but the encoder is slow which results into the encoding not catching up with the recording and thus filling the memory very fast. vp8 isn't really that much faster either but it supports multithreading, which when in use improves the performance significantly. We still have to figure out how many threads to use by default or whether we want to spilt the pipeline itself in multiple threads (the patch does the muxing in a separate thread and defaults to 4 encoding threads).
Created attachment 172757 [details] [review] recorder: Switch to webm The vp8 codec provides better performance in pretty much all cases compared to theora while still being free (as in not patent encumbered).
Comment on attachment 172757 [details] [review] recorder: Switch to webm Marking this needs-work to get it off the to-review list since it seemed from our IRC discussion we didn't want to go with this immediately (without widespread browser support for one thing)
Comment on attachment 172757 [details] [review] recorder: Switch to webm Putting it back on the review queue (as per todays IRC discussion), the only open question is what to do about the thread count.
Review of attachment 172757 [details] [review]: This seems fine. In terms of the number of threads, it's really how much time you want to spend on it. I think it would be fine if you hardcoded, say, 3 threads. (More than 3 unlikely to be useful. If there are less processors, hopefully scheduling won't leave the threads doing the UI too starved.) But without spending too much effort you could implement supporting, say, '%N' embedded in the string, with code along the lines of: #include <unistd.h> #ifdef _SC_NPROCESSORS_ONLN n_processors = sysconf(_SC_NPROCESSORS_ONLN); /* includes hyper-threading */ n_threads = MIN(1, n_processors - 1); #else n_threads = 3; #endif
Created attachment 178975 [details] [review] recorder: Switch to webm The vp8 codec provides better performance in pretty much all cases compared to theora while still being free (as in not patent encumbered). Also add a %T placeholder to the pipeline string which will be replaced with the a thread count based on the target system.
Review of attachment 178975 [details] [review]: ::: data/org.gnome.shell.gschema.xml.in @@ +103,3 @@ to an empty value, the default pipeline will be used. This is currently + 'videorate ! vp8enc quality=10 speed=2 threads=%T ! queue ! webmmux' + and records to WEBM using the VP8 codec. You might as well explain in a few words what %T means here. ::: src/shell-recorder.c @@ +1493,3 @@ + */ +static char* +set_thread_count (const char *pipeline) Weird function name substitute_thread_count is my suggested @@ +1508,3 @@ +#else + n_threads = 3; +#endif You are going to get a compile warning in the ndef case for unused variable, probably best to do #ifdef _SC_NPROCESSORS_ONLN { int n_processors = sysconf ... n_threads = .... } #else n_threads = 3; #endif @@ +1513,3 @@ + memset(buffer, 0, strlen (pipeline) + 1); + memcpy (buffer, pipeline, tmp - pipeline); + sprintf (buffer, "%s%d%s", buffer, n_threads, tmp + 2); This is pretty clumsy and needs thinking to understand. I'd do it as GString *result = g_string_new (NULL); g_string_append_len (result, pipeline, tmp - pipeline); g_string_append_printf (result, "%d" n_threads); g_string_append (result, tmp + 2); return g_string_free (result, FALSE); I'd also put in a comment somewhere like "Assume %T only occurs once"
(In reply to comment #6) > Review of attachment 178975 [details] [review]: > > ::: data/org.gnome.shell.gschema.xml.in > @@ +103,3 @@ > to an empty value, the default pipeline will be used. This is > currently > + 'videorate ! vp8enc quality=10 speed=2 threads=%T ! queue ! webmmux' > + and records to WEBM using the VP8 codec. > > You might as well explain in a few words what %T means here. OK > ::: src/shell-recorder.c > @@ +1493,3 @@ > + */ > +static char* > +set_thread_count (const char *pipeline) > > Weird function name > > substitute_thread_count > > is my suggested OK > @@ +1508,3 @@ > +#else > + n_threads = 3; > +#endif > > You are going to get a compile warning in the ndef case for unused variable, > probably best to do Indeed. > #ifdef _SC_NPROCESSORS_ONLN > { > int n_processors = sysconf ... > n_threads = .... > } > #else > n_threads = 3; > #endif > > @@ +1513,3 @@ > + memset(buffer, 0, strlen (pipeline) + 1); > + memcpy (buffer, pipeline, tmp - pipeline); > + sprintf (buffer, "%s%d%s", buffer, n_threads, tmp + 2); > > This is pretty clumsy and needs thinking to understand. I'd do it as > > GString *result = g_string_new (NULL); > g_string_append_len (result, pipeline, tmp - pipeline); > g_string_append_printf (result, "%d" n_threads); > g_string_append (result, tmp + 2); > > return g_string_free (result, FALSE); I'd also put in a comment somewhere like > "Assume %T only occurs once" OK
Created attachment 178987 [details] [review] recorder: Switch to webm The vp8 codec provides better performance in pretty much all cases compared to theora while still being free (as in not patent encumbered). Also add a %T placeholder to the pipeline string which will be replaced with the a thread count based on the target system.
Review of attachment 178987 [details] [review]: Looks good ::: data/org.gnome.shell.gschema.xml.in @@ +104,3 @@ + 'videorate ! vp8enc quality=10 speed=2 threads=%T ! queue ! webmmux' + and records to WEBM using the VP8 codec. %T is used as a placeholder + for the optimal thread count on the system. 'a guess at the optimal thread count" ?
(In reply to comment #9) > Review of attachment 178987 [details] [review]: > > Looks good > > ::: data/org.gnome.shell.gschema.xml.in > @@ +104,3 @@ > + 'videorate ! vp8enc quality=10 speed=2 threads=%T ! queue ! webmmux' > + and records to WEBM using the VP8 codec. %T is used as a placeholder > + for the optimal thread count on the system. > > 'a guess at the optimal thread count" ? OK
Attachment 178987 [details] pushed as c6baee2 - recorder: Switch to webm
*** Bug 610539 has been marked as a duplicate of this bug. ***