[cairo] SUMMARY of Implementing cairo_read_func_t/cairo_write_func_t properly

[Bernhard R. Fischer]

On 2016-03-04 19:16, Bryce Harrington wrote:
> On Thu, Mar 03, 2016 at 03:52:12PM +0100, Bernhard R. Fischer wrote:
>> On 2016-03-01 06:58, Bernhard R. Fischer wrote:
>>> The conclusion of this discussion was to slightly adapt the meaning of
>>> the return value of the read_func_t as follows:
>>>
>>>
>>> The read function shall return the negative number of bytes which have
>>> NOT been read, i.e. - (length - bytes_read).
>>> This is that a full read will return 0 which is CAIRO_STATUS_SUCCESS.
>>> Truncated reads return something between -1 and -length. On error,
>>> CAIRO_STATUS_READ_ERROR is returned (which is a positive value).
>>>
>>>
>>>
>>> If there is the consent of the core devs we should adapt the docs.
>>>
>>> I'll have a look at cairo_image_surface_create_from_png_stream() if
>>> there is a change necessary.
>>>
>>> Bernhard
>>>
>>
>>
>>
>> As promised, I just had a look at implementation of the PNG code of
>> Cairo (cairo-png.c).
>>
>> If a read_func() returns a negative value as suggested, the whole
>> program will be aborted because of a call to assert() in
>> _cairo_surface_create_in_error() (cairo-surface.c) which in turn is
>> called by the PNG error handler png_simple_error_callback() in cairo-png.c.
>>
>>
>> I suggest the following tiny patch (to cairo-1.14.6) to fix this:
>>
>>
>> *** cairo-png_orig.c    2016-03-03 15:41:35.565400515 +0100
>> --- cairo-png.c 2016-03-03 15:42:05.301547969 +0100
>> ***************
>> *** 525,530 ****
>> --- 525,531 ----
>>       png_closure = png_get_io_ptr (png);
>>       status = png_closure->read_func (png_closure->closure, data, size);
>>       if (unlikely (status)) {
>> +        if ((int) status < 0) status = CAIRO_STATUS_READ_ERROR;
>>         cairo_status_t *error = png_get_error_ptr (png);
>>         if (*error == CAIRO_STATUS_SUCCESS)
>>             *error = status;
> 
> This would probably need to look more like:
> 
>     status = png_closure->read_func (png_closure->closure, data, size);
>     if (unlikely (status)) {
>         cairo_status_t *error;
>         if ((int) status < 0)
>             status = CAIRO_STATUS_READ_ERROR;
>         error = png_get_error_ptr (png);


Why? It makes no difference.


> Was there also a discussion of instead of overloading the return value
> of the read_func_t to add a &size parameter to its arguments for
> returning the number of bytes read?


No, that was not suggested but of course this would be an option
although it would change the prototype of the function.
In this case we could define a new prototype instead and that was what I
originally proposed in this thread several weeks ago which was dropped.


Bernhard