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

On 07/29/2013 01:35 PM, Chris Wilson wrote:

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

Oof. Can't believe I missed that. Thanks for pointing it out.

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

It's true that this is mainly aimed at cairo_gl_surface_create, which is
public.

> 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.

If you are more comfortable with it, I can simply replicate the check at
the beginning of _cairo_gl_surface_create_similar and the associated
error surface. I think that's sufficient for what we need in WebKit.

> 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).

I've spent too much time writing tests for other harnesses. :) I'll fix
that.

-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo