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

[Tristan Matthews]

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
>> index b4d6ef4..7bb0320 100644
>> --- a/modules/codec/png.c
>> +++ b/modules/codec/png.c
>> @@ -49,6 +49,20 @@ static void CloseDecoder  ( vlc_object_t * );
>>
>>  static picture_t *DecodeBlock  ( decoder_t *, block_t ** );
>>
>> +/*
>> + * png encoder descriptor
>> + */
>> +struct encoder_sys_t
>> +{
>> +    int i_blocksize;
>> +    bool b_error;
>> +};
>> +
>> +static int  OpenEncoder(vlc_object_t *);
>> +static void CloseEncoder(vlc_object_t *);
>> +
>> +static block_t *EncodeBlock(encoder_t *, picture_t *);
>> +
>>  /*****************************************************************************
>>   * Module descriptor
>>   *****************************************************************************/
>> @@ -59,6 +73,14 @@ vlc_module_begin ()
>>      set_capability( "decoder", 1000 )
>>      set_callbacks( OpenDecoder, CloseDecoder )
>>      add_shortcut( "png" )
>> +
>> +    /* encoder submodule */
>> +    add_submodule()
>> +    add_shortcut("png")
>> +    set_section(N_("Encoding"), NULL)
>> +    set_description(N_("PNG video encoder"))
>> +    set_capability("encoder", 1000)
>> +    set_callbacks(OpenEncoder, CloseEncoder)
>>  vlc_module_end ()
>>
>>  /*****************************************************************************
>> @@ -101,19 +123,51 @@ static void user_read( png_structp p_png, png_bytep data, png_size_t i_length )
>>      p_block->i_buffer -= i_length;
>>  }
>>
>> +static void user_flush( png_structp p_png )
>> +{
>> +    /* noop */
>> +    (void) p_png;
>> +}
>> +
>> +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.

>
>> +        png_error( p_png, "not enough data" );
>> +        return;
>> +    }
>> +
>> +    memcpy( p_block->p_buffer, data, i_length );
>> +    p_block->p_buffer += i_length;
>> +    p_block->i_buffer -= i_length;
>> +}
>> +
>
>
>> -static void user_error( png_structp p_png, png_const_charp error_msg )
>> +static void user_decoder_error( png_structp p_png, png_const_charp error_msg )
>
> You can use the same function for both decoder and encoder, see my comments
> on encoder_error_message in
> https://mailman.videolan.org/pipermail/vlc-devel/2014-January/096466.html

Ah yes, it would be nice to get rid of some of those duplicate functions.

>
>>  {
>>      decoder_t *p_dec = (decoder_t *)png_get_error_ptr( p_png );
>>      p_dec->p_sys->b_error = true;
>>      msg_Err( p_dec, "%s", error_msg );
>>  }
>>
>> -static void user_warning( png_structp p_png, png_const_charp warning_msg )
>> +static void user_encoder_error( png_structp p_png, png_const_charp error_msg )
>> +{
>> +    encoder_t *p_enc = (encoder_t*)png_get_error_ptr( p_png );
>> +    p_enc->p_sys->b_error = true;
>> +    msg_Err( p_enc, "%s", error_msg );
>> +}
>> +
>> +static void user_decoder_warning( png_structp p_png, png_const_charp warning_msg )
>>  {
>>      decoder_t *p_dec = (decoder_t *)png_get_error_ptr( p_png );
>>      msg_Warn( p_dec, "%s", warning_msg );
>>  }
>>
>> +static void user_encoder_warning( png_structp p_png, png_const_charp warning_msg )
>> +{
>> +    encoder_t *p_enc = (encoder_t *)png_get_error_ptr( p_png );
>> +    msg_Warn( p_enc, "%s", warning_msg );
>> +}
>
> Same for these
>
>>  /****************************************************************************
>>   * DecodeBlock: the whole thing
>>   ****************************************************************************
>> @@ -172,7 +226,7 @@ static picture_t *DecodeBlock( decoder_t *p_dec, block_t **pp_block )
>>          goto error;
>>
>>      png_set_read_fn( p_png, (void *)p_block, user_read );
>> -    png_set_error_fn( p_png, (void *)p_dec, user_error, user_warning );
>> +    png_set_error_fn( p_png, (void *)p_dec, user_decoder_error, user_decoder_warning );
>>
>>      png_read_info( p_png, p_info );
>>      if( p_sys->b_error ) goto error;
>> @@ -253,3 +307,124 @@ static void CloseDecoder( vlc_object_t *p_this )
>>
>>      free( p_sys );
>>  }
>> +
>> +static int OpenEncoder(vlc_object_t *p_this)
>> +{
>> +    encoder_t *p_enc = (encoder_t *) p_this;
>> +
>> +    if( p_enc->fmt_out.i_codec != VLC_CODEC_PNG )
>> +        return VLC_EGENERIC;
>> +
>> +    /* Allocate the memory needed to store the encoder's structure */
>> +    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.

>
>> +    p_enc->p_sys->i_blocksize = 3 * p_enc->fmt_in.video.i_visible_width *
>> +        p_enc->fmt_in.video.i_visible_height;
>> +
>> +    p_enc->fmt_in.i_codec = VLC_CODEC_RGB24;
>> +    p_enc->fmt_in.video.i_rmask = 0x000000ff;
>> +    p_enc->fmt_in.video.i_gmask = 0x0000ff00;
>> +    p_enc->fmt_in.video.i_bmask = 0x00ff0000;
>> +    p_enc->pf_encode_video = EncodeBlock;
>> +
>> +    return VLC_SUCCESS;
>> +}
>> +
>> +/*
>> + * EncodeBlock
>> + */
>> +static block_t *EncodeBlock(encoder_t *p_enc, picture_t *p_pic)
>> +{
>> +    encoder_sys_t *p_sys = p_enc->p_sys;
>> +
>> +    if( unlikely( !p_pic ) )
>> +    {
>> +        return NULL;
>> +    }
>> +
>> +    block_t *p_block = block_Alloc( p_sys->i_blocksize );
>> +    if( p_block == NULL )
>> +    {
>> +        return NULL;
>> +    }
>> +
>> +    png_structp p_png = png_create_write_struct( PNG_LIBPNG_VER_STRING, 0, 0, 0 );
>> +    if( p_png == NULL )
>> +    {
>> +        block_Release( p_block );
>> +        return NULL;
>> +    }
>> +
>> +    /* save buffer start */
>> +    void *start = p_block->p_buffer;
>> +
>> +    p_sys->b_error = false;
>> +
>> +    /* libpng longjmp's there in case of error */
>> +    if( setjmp( png_jmpbuf( p_png ) ) )
>> +        goto error;
>> +
>> +    png_set_write_fn( p_png, p_block, user_write, user_flush );
>> +    png_set_error_fn( p_png, p_enc, user_encoder_error, user_encoder_warning );
>> +
>> +    png_infop p_info = png_create_info_struct( p_png );
>> +    if( p_info == NULL )
>> +    {
>
> goto error ?
>
> maybe you need a if (p_info) in error: case.

Yeah that would work.

>
>> +        png_destroy_write_struct( &p_png, NULL );
>> +        block_Release( p_block );
>> +        return NULL;
>> +    }
>> +
>> +    png_infop p_end_info = png_create_info_struct( p_png );
>> +    if( p_end_info == NULL ) goto error;
>> +
>> +    const unsigned i_width = p_enc->fmt_in.video.i_visible_width;
>> +    const unsigned i_height = p_enc->fmt_in.video.i_visible_height;
>> +
>> +    png_set_IHDR( p_png, p_info,
>> +            i_width,
>> +            i_height,
>> +            8, PNG_COLOR_TYPE_RGB, PNG_INTERLACE_NONE,
>> +            PNG_COMPRESSION_TYPE_DEFAULT, PNG_FILTER_TYPE_DEFAULT );
>> +    if( p_sys->b_error ) goto error;
>> +
>> +    png_write_info( p_png, p_info );
>> +    if( p_sys->b_error ) goto error;
>> +
>> +    /* Encode picture */
>> +
>> +    for( unsigned i = 0; i < i_height; i++ )
>> +    {
>> +        png_write_row( p_png, p_pic->p->p_pixels + (i_width * i * 3));
>> +        if( p_sys->b_error ) goto error;
>> +    }
>> +
>> +    png_write_end( p_png, p_end_info );
>> +    if( p_sys->b_error ) goto error;
>> +
>> +    png_destroy_write_struct( &p_png, &p_info );
>> +
>> +    /* 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.
Also, in case anyone was curious, I'm using png_write_rows since it
avoids having to malloc/free a set of row_pointers.

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

Best,
Tristan