[vlc-devel] [PATCH] png: add encoder
Tristan Matthews
le.businessman at gmail.com
Wed Feb 19 17:28:54 CET 2014
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
More information about the vlc-devel
mailing list