GNOME Bugzilla – Bug 609053
[recorder] Use GL_MESA_pack_invert if available
Last modified: 2010-02-05 21:27:56 UTC
Currently we use videoflip in our gstreamer pipeline, this can be avoided by using GL_MESA_pack_invert when available. GL_MESA_pack_invert isn't always available (ex: nvidia does not support it) so have the old code as a fallback.
Created attachment 153064 [details] [review] [recorder] Use GL_MESA_pack_invert if available
Created attachment 153068 [details] [review] [recorder] Use GL_MESA_pack_invert if available Minor style cleanups
Review of attachment 153068 [details] [review]: Thanks for working on this! Generally patch looks good, few comments follow. ::: src/shell-recorder.c @@ +248,3 @@ recorder->memory_target = get_memory_target(); + /* check whether GL_MESA_pack_invert is available */ This comment is probably extraneous :-) @@ +249,3 @@ + /* check whether GL_MESA_pack_invert is available */ + recorder->has_pack_invert = cogl_check_extension ("GL_MESA_pack_invert", (const char*) glGetString (GL_EXTENSIONS)); Calling glGetString (GL_EXTENSIONS) only works if the right GL context is active; it's probably not good to assume that that is the case outside of a paint function. There's not really a great way to do this that I'm aware of; I think what I'd do is put it next to the recorder->have_xfixes check in recorder_set_stage() and use: glx_extensions = glXQueryExtensionsString (clutter_x11_get_default_display (), clutter_x11_get_default_screen ()); Also, should be have_pack_invert to match have_xfixes. @@ +435,3 @@ cr = cairo_create (surface); + cairo_translate(cr, 0, recorder->has_pack_invert ? 0 : recorder->stage_height); + cairo_scale(cr, 1, recorder->has_pack_invert ? 1 : -1); This would be considerably clearer as: if (!recorder->has_pack_invert) { cairo_translate(cr, 0, recorder->stage_height); cairo_scale(cr, 1, -1); } Deciphering the fact that the translate/scale have no effect in the has_pack_invert case is hard. @@ +1087,3 @@ */ + + if (!cogl_check_extension ("GL_MESA_pack_invert", (const char*) glGetString (GL_EXTENSIONS))) This should use pipeline->recorder->have_pack_invert, right? @@ +1091,3 @@ + videoflip = gst_parse_launch_full ("videoflip method=vertical-flip", NULL, + GST_PARSE_FLAG_FATAL_ERRORS, + &error); Looks like this isn't quite lined up right @@ +1101,3 @@ + gst_bin_add (GST_BIN (pipeline->pipeline), videoflip); + gst_element_link_many (pipeline->src, ffmpegcolorspace, videoflip, + NULL); Again this doesn't look lined up
(In reply to comment #3) > Review of attachment 153068 [details] [review]: > > Thanks for working on this! Generally patch looks good, few comments follow. > > ::: src/shell-recorder.c > @@ +248,3 @@ > recorder->memory_target = get_memory_target(); > > + /* check whether GL_MESA_pack_invert is available */ > > This comment is probably extraneous :-) OK ;) > @@ +249,3 @@ > > + /* check whether GL_MESA_pack_invert is available */ > + recorder->has_pack_invert = cogl_check_extension ("GL_MESA_pack_invert", > (const char*) glGetString (GL_EXTENSIONS)); > > Calling glGetString (GL_EXTENSIONS) only works if the right GL context is > active; it's probably not good to assume that that is the case outside of a > paint function. > > There's not really a great way to do this that I'm aware of; I think what I'd > do is put it next to the recorder->have_xfixes check in recorder_set_stage() > and use: > > glx_extensions = > glXQueryExtensionsString (clutter_x11_get_default_display (), > clutter_x11_get_default_screen ()); Well either I am missing something or this wont work, GL_MESA_pack_invert is an opengl extension not a GLX one so glXQueryExtensionsString would never find it. > Also, should be have_pack_invert to match have_xfixes. OK, that makes sense. > @@ +435,3 @@ > cr = cairo_create (surface); > + cairo_translate(cr, 0, recorder->has_pack_invert ? 0 : > recorder->stage_height); > + cairo_scale(cr, 1, recorder->has_pack_invert ? 1 : -1); > > This would be considerably clearer as: > > if (!recorder->has_pack_invert) { > cairo_translate(cr, 0, recorder->stage_height); > cairo_scale(cr, 1, -1); > } > > Deciphering the fact that the translate/scale have no effect in the > has_pack_invert case is hard. True, this is indeed easier to read. > @@ +1087,3 @@ > */ > + > + if (!cogl_check_extension ("GL_MESA_pack_invert", (const char*) glGetString > (GL_EXTENSIONS))) > > This should use pipeline->recorder->have_pack_invert, right? Correct. > @@ +1091,3 @@ > + videoflip = gst_parse_launch_full ("videoflip method=vertical-flip", > NULL, > + GST_PARSE_FLAG_FATAL_ERRORS, > + &error); > > Looks like this isn't quite lined up right > [...] > Again this doesn't look lined up OK ... fill fix that up. Thanks for the review.
(In reply to comment #4) > > > @@ +249,3 @@ > > > > + /* check whether GL_MESA_pack_invert is available */ > > + recorder->has_pack_invert = cogl_check_extension ("GL_MESA_pack_invert", > > (const char*) glGetString (GL_EXTENSIONS)); > > > > Calling glGetString (GL_EXTENSIONS) only works if the right GL context is > > active; it's probably not good to assume that that is the case outside of a > > paint function. > > > > There's not really a great way to do this that I'm aware of; I think what I'd > > do is put it next to the recorder->have_xfixes check in recorder_set_stage() > > and use: > > > > glx_extensions = > > glXQueryExtensionsString (clutter_x11_get_default_display (), > > clutter_x11_get_default_screen ()); > > Well either I am missing something or this wont work, GL_MESA_pack_invert is an > opengl extension not a GLX one so glXQueryExtensionsString would never find it. Oops, right, use: clutter_stage_ensure_current (stage); gl_extensions = (const char *)glGetString (GL_EXTENSIONS); In shell_recorder_set_stage() - that's even clean...
Created attachment 153107 [details] [review] [recorder] Use GL_MESA_pack_invert if available Fixed up patch.
Review of attachment 153107 [details] [review]: Looks good, please commit.
Comment on attachment 153107 [details] [review] [recorder] Use GL_MESA_pack_invert if available Committed as c88d21d