Re: [PATCH]: gl: Return surface in error when creating oversized texture surfaces

On Mon, Jul 29, 2013 at 12:34:40PM -0700, Martin Robinson wrote:
> From 33d7b642d615d0d5d9b2792b9510a501a6b8a4f6 Mon Sep 17 00:00:00 2001
> From: Martin Robinson <[email protected]>
> Date: Mon, 29 Jul 2013 11:14:34 -0700
> Subject: [PATCH] gl: Return surface in error when creating oversized texture
>  surfaces
> 
> When creating a texture surface that is larger than the maximum
> framebuffer or texture dimensions of the context, return a surface in
> error. Previously the code failed an assertion, but this prevents an
> application from easily detecting when to fall back.

(Note there is already a function _cairo_gl_surface_size_valid, maybe
extend that to handle read-only/write-only/read-write surfaces?)

The majority of its callers do seem to be internal and only expect a gl
surface to be created.

I feel a little uneasy about changing the user API though to error out
if asked to handle a surface too large for hw. In most cases it is
better to be upfront about when they are going to encounter pain, but on
the other hand we have a long history of hiding such hardware details.
(Though I thought we did expose the limits through queries on the
cairo_gl_context_t device.) The problem is that though the hardware may
only support 8k coordinates, it may be be sample from and render to
textures 64k x 64k in size (and technically even larger), for instance
Intel hw. Whether or not OpenGL is ever capable of expressing such
operations and their limits sanely is another question. But imposing
this limit feels like we are artificially constraining the hw and
preventing common use cases.

This patch also needs to extend (create!) the documentation for
cairo_gl_surface_create() explaining the various limits and how to
implement a fallback, such as

  surface = cairo_gl_surface_create(device, content, width, height);
  if (cairo_surface_status(surface) == CAIRO_STATUS_INVALID_SIZE)
     surface = cairo_image_surface_create(choose_format_from_content(content),
                                          width, height);

and perhaps remind the reader that cairo_surface_create_similar() will
do a similar substitution automatically.

> +    oversized_surface = cairo_gl_surface_create (device, CAIRO_CONTENT_COLOR_ALPHA, INT_MAX, INT_MAX);
> +    assert (cairo_surface_status (oversized_surface) == CAIRO_STATUS_INVALID_SIZE);

This should just set the test_status = CAIRO_TEST_FAILURE and keep going.
So that we don't rely on the test process being forked (i.e. under
gdb and valgrind etc).

Except for the part about cairo's API is meant to hide hardware details
where pragmatic. Although as INT_MAX > 2<<24, that's a fair error
considering the current code!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo