Include GL extension fliter/allowlist if and only if its needed

[sbc100]

Here we use the JS library dependency mechanism to include this code only when its needed.

Sadly this will end up being present in almost all GL-using programs because most of them will
end up using emscriptenWebGLGet (e.g. any app that calls glGetInteger, glGetString, etc).
The code size impact of this change (for codebases that call glGetString but don't otherwise
call GetProcAddress is 858 bytes of JS or 356 compressed).

Fixes: #20798

[sbc100]

I measured the code size impact of this change for codebases that call glGetString but don't call GetProcAddress as 858 bytes of JS or 356 compressed JS. @juj, f you think we need a switch to make this less conservative we could add one perhaps.

[kainino0x]

LGTM

[juj]

Sorry, I don't agree with this PR. We should not land this, see #20798 (comment) for rationale.

LGTM to land if there is an opt-out for developers who statically know their code will not cause problems.

[juj]

Although, won't this still leave this code path unattended?

emscripten/src/library_webgl.js

Lines 1205 to 1215 in 53ce9c7

	// .getSupportedExtensions() can return null if context is lost, so coerce
	// to empty array.
	var exts = GLctx.getSupportedExtensions() || [];
	exts.forEach((ext) => {
	// WEBGL_lose_context, WEBGL_debug_renderer_info and WEBGL_debug_shaders
	// are not enabled by default.
	if (!ext.includes('lose_context') && !ext.includes('debug')) {
	// Call .getExtension() to enable that extension permanently.
	GLctx.getExtension(ext);
	}
	});

[sbc100]

Although, won't this still leave this code path unattended?

Ah yes, I misunderstood that point of that callsite and assumed it wanted the unfiltered list

[sbc100]

Hopefully this address all the concern and can finally close #20798. Will wait for @juj to get back from vacation for final approval.

will defer final review to juj

[kainino0x]

This is getting a bit deep into things for me. Since Jukka will have to review anyway I will just defer to him. Few small comments though.

[juj]

LGTM

[floooh]

Could this change have broken the enableExtensionsByDefault behaviour of the emscripten_webgl_create_context() call? Since emsdk 3.1.52 I'm no longer seeing the WEBGL_compressed_texture_etc and WEBGL_compressed_texture_astc extension advertised via glGetStringi(GL_EXTENSIONS,...).

...maybe they are just missing in this list?

emscripten/src/library_webgl.js

Lines 140 to 183 in c4d76b8

	// Restrict the list of advertised extensions to those that we actually
	// support.
	var supportedExtensions = [
	#if MIN_WEBGL_VERSION == 1
	// WebGL 1 extensions
	'ANGLE_instanced_arrays',
	'EXT_blend_minmax',
	'EXT_disjoint_timer_query',
	'EXT_frag_depth',
	'EXT_shader_texture_lod',
	'EXT_sRGB',
	'OES_element_index_uint',
	'OES_fbo_render_mipmap',
	'OES_standard_derivatives',
	'OES_texture_float',
	'OES_texture_half_float',
	'OES_texture_half_float_linear',
	'OES_vertex_array_object',
	'WEBGL_color_buffer_float',
	'WEBGL_depth_texture',
	'WEBGL_draw_buffers',
	#endif
	#if MAX_WEBGL_VERSION >= 2
	// WebGL 2 extensions
	'EXT_color_buffer_float',
	'EXT_disjoint_timer_query_webgl2',
	'EXT_texture_norm16',
	'WEBGL_clip_cull_distance',
	#endif
	// WebGL 1 and WebGL 2 extensions
	'EXT_color_buffer_half_float',
	'EXT_float_blend',
	'EXT_texture_compression_bptc',
	'EXT_texture_compression_rgtc',
	'EXT_texture_filter_anisotropic',
	'KHR_parallel_shader_compile',
	'OES_texture_float_linear',
	'WEBGL_compressed_texture_s3tc',
	'WEBGL_compressed_texture_s3tc_srgb',
	'WEBGL_debug_renderer_info',
	'WEBGL_debug_shaders',
	'WEBGL_lose_context',
	'WEBGL_multi_draw',
	];

PS: explicitly calling:

    emscripten_webgl_enable_extension(ctx, "WEBGL_compressed_texture_etc");
    emscripten_webgl_enable_extension(ctx, "WEBGL_compressed_texture_astc");

...also doesn't seem to make those extensions visible via glGetStringi() (this used to be a workaround in the past to get access to the PVRTC extension, since this wasn't automatically enabled by Emscripten).

[kainino0x]

That is probably the problem. I thought I checked that this list had all the extensions I expected it to have from https://registry.khronos.org/webgl/extensions/, but I may have forgotten to check the community approved extensions in addition to the ratified ones.

[sbc100]

That is probably the problem. I thought I checked that this list had all the extensions I expected it to have from https://registry.khronos.org/webgl/extensions/, but I may have forgotten to check the community approved extensions in addition to the ratified ones.

It this PR in particular? I assume if there are issues with the contents of the list then #20802 would be the root cause. This PR was mostly refactoring.

[kainino0x]

Right, I didn't check which PR this was. I think it must have been that one.

[floooh]

Thanks for looking into this, and apologies too for identifying the wrong PR.