758844 - Quiet graphics code during startup

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

People who aren't debugging graphics code don't need this on every startup. It gets in the way of seeing interesting things like warnings.

OpenGL vendor ('ATI Technologies Inc.') recognized as: ATI
GL extensions: GL_ARB_color_buffer_float GL_ARB_depth_buffer_float GL_ARB_depth_clamp GL_ARB_depth_texture GL_ARB_draw_buffers GL_ARB_draw_elements_base_vertex GL_ARB_draw_instanced GL_ARB_fragment_program GL_ARB_fragment_program_shadow GL_ARB_fragment_shader GL_ARB_framebuffer_object GL_ARB_framebuffer_sRGB GL_ARB_half_float_pixel GL_ARB_half_float_vertex GL_ARB_imaging GL_ARB_instanced_arrays GL_ARB_multisample GL_ARB_multitexture GL_ARB_occlusion_query GL_ARB_pixel_buffer_object GL_ARB_point_parameters GL_ARB_point_sprite GL_ARB_provoking_vertex GL_ARB_seamless_cube_map GL_ARB_shader_objects GL_ARB_shader_texture_lod GL_ARB_shading_language_100 GL_ARB_shadow GL_ARB_shadow_ambient GL_ARB_sync GL_ARB_texture_border_clamp GL_ARB_texture_compression GL_ARB_texture_compression_rgtc GL_ARB_texture_cube_map GL_ARB_texture_env_add GL_ARB_texture_env_combine GL_ARB_texture_env_crossbar GL_ARB_texture_env_dot3 GL_ARB_texture_float GL_ARB_texture_mirrored_repeat GL_ARB_texture_non_power_of_two GL_ARB_texture_rectangle GL_ARB_texture_rg GL_ARB_transpose_matrix GL_ARB_vertex_array_bgra GL_ARB_vertex_blend GL_ARB_vertex_buffer_object GL_ARB_vertex_program GL_ARB_vertex_shader GL_ARB_window_pos GL_EXT_abgr GL_EXT_bgra GL_EXT_bindable_uniform GL_EXT_blend_color GL_EXT_blend_equation_separate GL_EXT_blend_func_separate GL_EXT_blend_minmax GL_EXT_blend_subtract GL_EXT_clip_volume_hint GL_EXT_draw_buffers2 GL_EXT_draw_range_elements GL_EXT_fog_coord GL_EXT_framebuffer_blit GL_EXT_framebuffer_multisample GL_EXT_framebuffer_object GL_EXT_framebuffer_sRGB GL_EXT_geometry_shader4 GL_EXT_gpu_program_parameters GL_EXT_gpu_shader4 GL_EXT_multi_draw_arrays GL_EXT_packed_depth_stencil GL_EXT_packed_float GL_EXT_provoking_vertex GL_EXT_rescale_normal GL_EXT_secondary_color GL_EXT_separate_specular_color GL_EXT_shadow_funcs GL_EXT_stencil_two_side GL_EXT_stencil_wrap GL_EXT_texture_array GL_EXT_texture_compression_dxt1 GL_EXT_texture_compression_s3tc GL_EXT_texture_env_add GL_EXT_texture_filter_anisotropic GL_EXT_texture_integer GL_EXT_texture_lod_bias GL_EXT_texture_mirror_clamp GL_EXT_texture_rectangle GL_EXT_texture_shared_exponent GL_EXT_texture_sRGB GL_EXT_texture_sRGB_decode GL_EXT_timer_query GL_EXT_transform_feedback GL_EXT_vertex_array_bgra GL_APPLE_aux_depth_stencil GL_APPLE_client_storage GL_APPLE_element_array GL_APPLE_fence GL_APPLE_float_pixels GL_APPLE_flush_buffer_range GL_APPLE_flush_render GL_APPLE_object_purgeable GL_APPLE_packed_pixels GL_APPLE_pixel_buffer GL_APPLE_rgb_422 GL_APPLE_row_bytes GL_APPLE_specular_vector GL_APPLE_texture_range GL_APPLE_transform_hint GL_APPLE_vertex_array_object GL_APPLE_vertex_array_range GL_APPLE_vertex_point_size GL_APPLE_vertex_program_evaluators GL_APPLE_ycbcr_422 GL_ATI_blend_equation_separate GL_ATI_blend_weighted_minmax GL_ATI_separate_stencil GL_ATI_texture_compression_3dc GL_ATI_texture_env_combine3 GL_ATI_texture_float GL_ATI_texture_mirror_once GL_IBM_rasterpos_clip GL_NV_blend_square GL_NV_conditional_render GL_NV_depth_clamp GL_NV_fog_distance GL_NV_light_max_exponent GL_NV_texgen_reflection GL_SGI_color_matrix GL_SGIS_generate_mipmap GL_SGIS_texture_edge_clamp GL_SGIS_texture_lod
Found extension GL_ARB_framebuffer_object
Found extension GL_ARB_pixel_buffer_object
Found extension GL_ARB_sync
Found extension GL_ARB_texture_float
Found extension GL_ARB_texture_non_power_of_two
Found extension GL_ARB_texture_rectangle
Found extension GL_EXT_bgra
Found extension GL_EXT_framebuffer_blit
Found extension GL_EXT_framebuffer_multisample
Found extension GL_EXT_framebuffer_object
Found extension GL_EXT_texture_compression_s3tc
Found extension GL_EXT_texture_filter_anisotropic
Found extension GL_APPLE_client_storage

Comment 1

[Kelsey Gilbert [:jgilbert]]

Agreed, this should be hidden behind MOZ_GL_DEBUG.

Comment 2

[John Drinkwater (:beta)]

Attachment: Disable debug output unless MOZ_GL_DEBUG is defined

A stab at fixing this, my first patch.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 629200 [details] [diff] [review]
Disable debug output unless MOZ_GL_DEBUG is defined

Review of attachment 629200 [details] [diff] [review]:
-----------------------------------------------------------------

Don't re-read the env var everytime. We already read it, into a |mDebugMode| bitfield, so just check for mDebugMode != 0. If the env var hasn't been read yet when you need it, try shuffling things around.

Comment 4

[John Drinkwater (:beta)]

Attachment: Disable debug output unless MOZ_GL_DEBUG is defined

Changed per review comment, using sDebugMode.
Moved the sDebugMode mask code‐block further up in the InitWithPrefix function so it is usable.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 629241 [details] [diff] [review]
Disable debug output unless MOZ_GL_DEBUG is defined

Review of attachment 629241 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple of nits.

Also, I'm scared that this is playing with the value of |once| which could be tricky in the future. But this is OK as this whole function is a kludge and needs to be refactored completely.

::: gfx/gl/GLContext.cpp
@@ +328,4 @@
>      if (mInitialized) {
>  #ifdef DEBUG
>          static bool once = false;
> +        if ( sDebugMode != 0 ) {

No spaces around the condition inside of the (), and !=0 is useless, so I would write: if (sDebugMode).

@@ +526,5 @@
>      char *exts = strdup((char *)extensions);
>  
>  #ifdef DEBUG
>      static bool once = false;
> +    if ( sDebugMode != 0 ) {

Same.

Comment 6

[John Drinkwater (:beta)]

Attachment: Disable debug output unless MOZ_GL_DEBUG is defined

Fixed spacing and conditional.

Comment 7

[Jesse Ruderman]

I still see the spew on Try:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12322065&tree=Try

Maybe the conditional is backwards?

Comment 8

[John Drinkwater (:beta)]

(In reply to Jesse Ruderman from comment #7)
> I still see the spew on Try:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=12322065&tree=Try
> 
> Maybe the conditional is backwards?

Yeah gosh, thought I’d tested that. New patch coming.

Comment 9

[John Drinkwater (:beta)]

Attachment: Disable most WEBGL debug output unless env vars set.

Suppresses WebGL init, context messages and outstanding buffers.

Comment 10

[Jesse Ruderman]

$ hg qimport ~/Desktop/bug-758844d.patch
$ hg qrefresh -u "John Drinkwater <[e-mail]>" -m "Bug 758844 - Disable debug output unless MOZ_GL_DEBUG is defined. r?bjacob. (try: -b do -p all -u reftest,reftest-1 -t none)"
$ hg try

Results will appear at https://tbpl.mozilla.org/?rev=1fbecdfc6bfd&tree=Try

Comment 11

[John Drinkwater (:beta)]

Try failed on Android due to unrelated changeset https://hg.mozilla.org/try/rev/762be4801739

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 630636 [details] [diff] [review]
Disable most WEBGL debug output unless env vars set.

Review of attachment 630636 [details] [diff] [review]:
-----------------------------------------------------------------

I think that's all of them, though I didn't actually check. But the ones you touch on here are done right.

Eventually, we'd like to drop the #ifdef DEBUG tags around things, since DebugMode() should get inlined to |false|, which should optimized out. However, we haven't check this, so are erring on the side of caution and leaving the #ifdefs for now.

Comment 13

[Jesse Ruderman]

Is this good to push, or should we wait for bjacob to r+ the new patch as well?

Comment 14

[John Drinkwater (:beta)]

I would have thought its good to go.

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Jeff's r+ is definitely all you need!

Comment 16

[Jesse Ruderman]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29d9f71b24ac

Comment 17

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/29d9f71b24ac

Comment 18

[Jesse Ruderman]

Bug 762265 effectively reverted this change without discussion.  I am unhappy about that.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Ouch, yeah, bug collision. This was not at all the intent of bug 762265.

Comment 20

[John Drinkwater (:beta)]

Attachment: Revert some changes from Bug 762265

No worries, this should undo the change in Bug 762265.
Could you put this through a try please, as I cannot test output from EGL here.

Comment 21

[Jesse Ruderman]

https://tbpl.mozilla.org/?tree=Try&rev=996ba2a2ee4b

Comment 22

[Kelsey Gilbert [:jgilbert]]

Attachment: Quiet GFX spew except on the first run in DebugMode()

This ties in the above changes, does a tiny bit of cleanup, and tries to make the logic less convoluted.

Comment 23

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0888a88ab4c7

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0888a88ab4c7