PDA

View Full Version : Should glReadNPixels()'s bufSize check count in pixel store parameters?



samwyi
09-11-2017, 04:04 PM
I'm implementing glReadNPixels() in a driver. this API has a bufSize parameter. GLES spec 3.2 says: "An INVALID_OPERATION error is generated by ReadnPixels if the buffersize required to store the requested data is greater than bufSize".

Here is my question:
When the driver calculates the required buffer size, should we count in the pixel store parameters set by glPixelStore(), e.g. PACK_ROW_LENGTH or PACK_SKIP_PIXELS? Or we simply check width*height*pixel_size? The required buffer size would be larger if the app set PACK_ROW_LENGTH to a value larger than pixel rectangle width, right?

Thanks.

Dark Photon
09-12-2017, 06:39 AM
When the driver calculates the required buffer size, should we count in the pixel store parameters set by glPixelStore(), e.g. PACK_ROW_LENGTH or PACK_SKIP_PIXELS? Or we simply check width*height*pixel_size?

It appears that "bufsize" is intended to allow the driver to sanity-check the write it will do against an app-provided buffer size, avoiding an obvious array overrun. So I suspect it should account for all PACK state in computing "min_byte_offset_to_write" and "max_byte_offset_to_write", and then verify that bufSize >= (max+1).

This is consistent with the error condition, as "the buffer size required to store the requested data" may be greater than the minimum number of bytes to be written within that buffer.

That said, I'm not a spec language lawyer. Hopefully one will follow-up.

GClements
09-12-2017, 06:46 AM
When the driver calculates the required buffer size, should we count in the pixel store parameters set by glPixelStore()

Yes. Those parameters determine the layout (and thus the size) of the returned data. If that's larger than the bufSize parameter, an error should be generated. That's the whole point of having glReadnPixels(): it provides a safety check against the client providing a buffer that's too small for the returned data.

It isn't immediately clear from either the reference page or the specification whether any trailing padding arising from GL_PACK_ROW_LENGTH or GL_PACK_ALIGNMENT should be counted towards the "required" buffer size. I would tend to assume that at least GL_PACK_ALIGNMENT can be taken into account; otherwise the driver wouldn't be able to safely use whole-word copy operations. You might want to check how existing drivers handle this situation.

samwyi
09-12-2017, 12:23 PM
It isn't immediately clear from either the reference page or the specification whether any trailing padding arising from GL_PACK_ROW_LENGTH or GL_PACK_ALIGNMENT should be counted towards the "required" buffer size. I would tend to assume that at least GL_PACK_ALIGNMENT can be taken into account; otherwise the driver wouldn't be able to safely use whole-word copy operations.

Yes, these are unclear in the spec. Good point on GL_PACK_ALIGNMENT, the driver better take this into account.

samwyi
09-12-2017, 01:13 PM
So I guess the purpose of introducing glReadNPixels is to give the app a chance to specify the buffer size when reading pixels to client memory. For PBOs, the buffer size is specified in glBufferData. For non-PBO case, we need this new API.