[vlc-devel] [PATCH] png: add encoder

[Rafaël Carré]

On 02/19/14 17:28, Tristan Matthews wrote:
> Hi, thanks for the speedy review, responses inline:
> 
> On Wed, Feb 19, 2014 at 2:39 AM, Rafaël Carré <funman at videolan.org> wrote:
>> Hello, a few comments below
>>
>> On 02/19/14 08:27, Tristan Matthews wrote:
>>> This patch adds a basic encoder to the png module (using libpng).
>>>
>>> ---
>>>  modules/codec/png.c | 181 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 178 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/modules/codec/png.c b/modules/codec/png.c

>>> +static void user_write( png_structp p_png, png_bytep data, png_size_t i_length )
>>> +{
>>> +    block_t *p_block = (block_t *)png_get_io_ptr( p_png );
>>> +    if( i_length > p_block->i_buffer ) {
>>
>> can you use block_Realloc and store the result ?
> 
> Yeah that should be doable, but if the correct size hasn't been
> allocated up-front
> (in EncodeBlock) that seems like a programming error.

Makes sense.

>>> +        png_error( p_png, "not enough data" );

Can you add i_length / p_block->i_buffer to help debug if this problem
arises?

>>> +        return;
>>> +    }
>>> +
>>> +    memcpy( p_block->p_buffer, data, i_length );
>>> +    p_block->p_buffer += i_length;
>>> +    p_block->i_buffer -= i_length;
>>> +}
>>> +

>>> +    if( ( p_enc->p_sys = malloc(sizeof(encoder_sys_t)) ) == NULL )
>>> +        return VLC_ENOMEM;
>>
>> Honestly I try to remove assignements and comparisons merged together
>> when I see them.
> 
> Ok, I could also do that for the decoder_sys_t as well in a separate commit.

Would be nice :)


>>> +    /* restore original buffer position */
>>> +    p_block->p_buffer = start;
>>
>> Why would it be modified?
> 
> In user_write. Now I realize that I should also be restoring i_buffer.

Ah I see, thanks.

> Also, in case anyone was curious, I'm using png_write_rows since it
> avoids having to malloc/free a set of row_pointers.

Does it mean we can do the same for jpeg? O:-)

>>> +    return p_block;
>>> +
>>> + error:
>>> +
>>> +    png_destroy_write_struct( &p_png, &p_info );
>>> +
>>> +    p_block->p_buffer = start;
>>
>> Not needed if the block is freed.
> 
> Ok, wasn't sure about that.
> 
> Hopefully I'll resend tonight.

Thanks,

> Best,
> Tristan