[vlc-devel] [PATCH 1/1] codec/dirac: Rewrite libdirac(-research) encoding support
Laurent Aimar
fenrir at via.ecp.fr
Mon Nov 10 22:57:30 CET 2008
Hi,
> +#define ENC_PREFILTER "prefilter"
> +#define ENC_PREFILTER_TEXT N_("Prefilter")
> +#define ENC_PREFILTER_LONGTEXT N_("Enable adaptive prefiltering")
> +static const char *const enc_prefilter_list[] =
> + { "none", "cwm", "rectlp", "diaglp" };
> +static const char *const enc_prefilter_list_text[] =
> + { N_("none"), N_("cwm"), N_("rectlp"), N_("diaglp") };
Could you change them to what they mean ?
(not blocking).
> +#define ENC_CHROMAFMT "chroma-fmt"
> +#define ENC_L1SEP "l1-sep"
> +#define ENC_L1NUM "num-l1"
I am not in favor of shortcut option names but I may be the only
one (and mainly when not abvious shortcut).
(not blocking).
> + add_integer( ENC_CFG_PREFIX ENC_TARGETRATE, -1, NULL,
> + ENC_TARGETRATE_TEXT, ENC_TARGETRATE_LONGTEXT, false );
> + change_integer_range(0, INT_MAX);
I am not sure but using a default value of -1 and then a range of [0, >0]
is probably buggy (and it may depends weither your conf file has been saved or
not).
(There is a lot of options in this case).
> +/*****************************************************************************
> + * picture_pts_t : store pts alongside picture number, not carried through
> + * encoder
> + *****************************************************************************/
> +struct picture_pts_t
> +{
> + bool i_empty; /* entry is invalid */
b_ iof i_
> +/*****************************************************************************
> + * ResetPTStlb: Purge all entries in @p_dec@'s PTS-tlb
> + *****************************************************************************/
> +static void ResetPTStlb( encoder_t *p_enc )
> +{
> + encoder_sys_t *p_sys = p_enc->p_sys;
> + for( int i=0; i<PTS_TLB_SIZE; i++)
> + {
> + p_sys->pts_tlb[i].i_empty = 1;
true iof 1.
> +/*****************************************************************************
> + * StorePicturePTS: Store the PTS value for a particular picture number
> + *****************************************************************************/
> +static void StorePicturePTS( encoder_t *p_enc, uint32_t u_pnum, mtime_t i_pts )
> +{
> + encoder_sys_t *p_sys = p_enc->p_sys;
> +
> + for( int i=0; i<PTS_TLB_SIZE; i++ )
> + {
> + if( p_sys->pts_tlb[i].i_empty )
> + {
> + p_sys->pts_tlb[i].u_pnum = u_pnum;
> + p_sys->pts_tlb[i].i_pts = i_pts;
> + p_sys->pts_tlb[i].i_empty = 0;
false iof 0.
> +/*****************************************************************************
> + * GetPicturePTS: Retrieve the PTS value for a particular picture number
> + *****************************************************************************/
> +static mtime_t GetPicturePTS( encoder_t *p_enc, uint32_t u_pnum )
> +{
> + encoder_sys_t *p_sys = p_enc->p_sys;
> +
> + for( int i=0; i<PTS_TLB_SIZE; i++ )
> + {
> + if( !p_sys->pts_tlb[i].i_empty &&
> + p_sys->pts_tlb[i].u_pnum == u_pnum )
> + {
> + p_sys->pts_tlb[i].i_empty = 1;
Idem.
> /*****************************************************************************
> * OpenEncoder: probe the encoder and return score
> *****************************************************************************/
> @@ -94,8 +437,9 @@ static int OpenEncoder( vlc_object_t *p_this )
> {
> /* Allocate the memory needed to store the decoder's structure */
> - if( ( p_sys = (encoder_sys_t *)malloc(sizeof(encoder_sys_t)) ) == NULL )
> + if( ( p_sys = calloc( 1, sizeof(*p_sys) ) ) == NULL )
> return VLC_ENOMEM;
> - memset( p_sys, 0, sizeof(encoder_sys_t) );
> - p_enc->p_sys = p_sys;
>
> + if( ( p_sys->p_dts_fifo = block_FifoNew() ) == NULL )
> + {
> + CloseEncoder( p_this );
^
> + return VLC_ENOMEM;
> + }
> +
> + p_enc->p_sys = p_sys;
^
You cannot call CloseEncoder before that line.
> + p_sys->ctx.src_params.frame_rate.numerator = p_enc->fmt_in.video.i_frame_rate;
> + p_sys->ctx.src_params.frame_rate.denominator = p_enc->fmt_in.video.i_frame_rate_base;
> + unsigned u_asr_num, u_asr_den;
> + vlc_ureduce( &u_asr_num, &u_asr_den,
> + p_enc->fmt_in.video.i_height * p_enc->fmt_in.video.i_aspect,
> + p_enc->fmt_in.video.i_width * VOUT_ASPECT_FACTOR,
> + 0 );
> + p_sys->ctx.src_params.pix_asr.numerator = u_asr_num;
> + p_sys->ctx.src_params.pix_asr.denominator = u_asr_den;
> +
> + config_ChainParse( p_enc, ENC_CFG_PREFIX, ppsz_enc_options, p_enc->p_cfg );
> +
> + psz_tmp = var_GetString( p_enc, ENC_CFG_PREFIX ENC_CHROMAFMT );
> + if( 0 ) {}
> + else if( !strcmp( psz_tmp, "420" ) ) {
> + p_enc->fmt_in.i_codec = VLC_FOURCC('I','4','2','0');
> + p_enc->fmt_in.video.i_bits_per_pixel = 12;
> + p_sys->ctx.src_params.chroma = format420;
> + p_sys->i_buffer_in = p_enc->fmt_in.video.i_width * p_enc->fmt_in.video.i_height * 3 / 2;
> + }
> + else if( !strcmp( psz_tmp, "422" ) ) {
> + p_enc->fmt_in.i_codec = VLC_FOURCC('I','4','2','2');
> + p_enc->fmt_in.video.i_bits_per_pixel = 16;
> + p_sys->ctx.src_params.chroma = format422;
> + p_sys->i_buffer_in = p_enc->fmt_in.video.i_width * p_enc->fmt_in.video.i_height * 2;
> + }
> + else if( !strcmp( psz_tmp, "444" ) ) {
> + p_enc->fmt_in.i_codec = VLC_FOURCC('I','4','4','4');
> + p_enc->fmt_in.video.i_bits_per_pixel = 24;
> + p_sys->ctx.src_params.chroma = format444;
> + p_sys->i_buffer_in = p_enc->fmt_in.video.i_width * p_enc->fmt_in.video.i_height * 3;
> + }
> + else {
> + msg_Err( p_enc, "Invalid chroma format: %s", psz_tmp );
> + free( psz_tmp );
> + goto error;
> + }
psz_tmp may be NULL in case of allocation failure.
Beside it can either be NULL or "" (empty) for empty/non set options.
I think you may want to use var_GetNonEmptyString (ie it will returns NULL
iof "") and fix the test.
(Idem for the following calls to var_GetString)
> + p_sys->ctx.enc_params.qf = var_GetFloat( p_enc, ENC_CFG_PREFIX ENC_QUALITY_FACTOR );
> +
> + /* use bitrate from sout-transcode-vb in kbps */
> + p_sys->ctx.enc_params.trate = p_enc->fmt_out.i_bitrate / 1000;
> + i_tmp = var_GetInteger( p_enc, ENC_CFG_PREFIX ENC_TARGETRATE );
> + if( i_tmp > -1 )
> + p_sys->ctx.enc_params.trate = i_tmp;
What will happens with 0 (may be ok)?
> + psz_tmp = var_GetString( p_enc, ENC_CFG_PREFIX ENC_ME_SIMPLESEARCH );
> + if( *psz_tmp != '\0' ) {
> + /* of the form [0-9]+:[0-9]+ */
> + char *psz_start = psz_tmp;
> + char *psz_end = psz_tmp;
> + p_sys->ctx.enc_params.x_range_me = strtol(psz_start, &psz_end, 10);
> + if( *psz_end != ':' || psz_end == psz_tmp ) {
^
Using psz_start may be easier to understand (and more consistant with next
test).
(not blocking)
> /****************************************************************************
> * Encode: the whole thing
> ****************************************************************************
> - * This function spits out ogg packets.
> + * This function spits out encapsulation units.
> ****************************************************************************/
> static block_t *Encode( encoder_t *p_enc, picture_t *p_pic )
> {
> +
> + /* Copy input picture into encoder input buffer (stride by stride) */
> + /* Would be lovely to just pass the picture in, but there is noway for the
> + * library to free it */
> p_dst = p_sys->p_buffer_in;
> for( i_plane = 0; i_plane < p_pic->i_planes; i_plane++ )
> {
> @@ -176,59 +836,127 @@ static block_t *Encode( encoder_t *p_enc, picture_t *p_pic )
>
> /* Load one frame of data into encoder */
> if( dirac_encoder_load( p_sys->p_dirac, p_sys->p_buffer_in,
> - p_sys->i_buffer_in ) >= 0 )
> + p_sys->i_buffer_in ) < 0 )
> + {
> + msg_Dbg( p_enc, "dirac_encoder_load() error" );
> + return NULL;
> + }
> +
> + /* store pts in a lookaside buffer, so that the same pts may
> + * be used for the picture in coded order */
> + StorePicturePTS( p_enc, p_sys->i_input_picnum, p_pic->date );
> + p_sys->i_input_picnum++;
> +
> + /* store dts in a queue, so that they appear in order in
> + * coded order */
> + p_block = block_New( p_enc, 1 );
> + p_block->i_dts = p_pic->date - p_sys->i_pts_offset;
> + block_FifoPut( p_sys->p_dts_fifo, p_block );
Missing test on p_block.
> + dirac_encoder_state_t state;
> + /* Retrieve encoded frames from encoder */
> + do
> + {
> + p_sys->p_dirac->enc_buf.buffer = p_sys->p_buffer_out;
> + p_sys->p_dirac->enc_buf.size = p_sys->i_buffer_out;
> + state = dirac_encoder_output( p_sys->p_dirac );
> + switch( state )
> {
> + case ENC_STATE_AVAIL: {
> + uint32_t pic_num;
> +
> + /* extract data from encoder temporary buffer. */
There, if p_sys->p_dirac->enc_buf.size >= p_sys->p_dirac I think you can
warn about (probable) output buffer overflow.
> + p_block = block_New( p_enc, p_sys->p_dirac->enc_buf.size );
> + if( !p_block )
> {
> + p_enc->b_error = 1;
> + return NULL;
> }
> - if( p_sys->p_dirac->decoded_frame_avail )
> + memcpy( p_block->p_buffer, p_sys->p_dirac->enc_buf.buffer,
> + p_sys->p_dirac->enc_buf.size );
> +
> + /* if some flags were set for a previous block, prevent
> + * them from getting lost */
> + if( p_sys->p_chain )
> + p_block->i_flags |= p_sys->p_chain->i_flags;
> +
> + /* store all extracted blocks in a chain and gather up when an
> + * entire encapsulation unit is avaliable (ends with a picture) */
> + block_ChainAppend( &p_sys->p_chain, p_block );
> +
> + /* Presence of a Sequence header indicates a seek point */
> + if( 0 == p_block->p_buffer[4] )
> {
> + p_block->i_flags |= BLOCK_FLAG_TYPE_I;
> +
> + if( !p_enc->fmt_out.p_extra ) {
> + const uint8_t eos[] = { 'B','B','C','D',0x10,0,0,0,13,0,0,0,0 };
> + uint32_t len = GetDWBE( p_block->p_buffer + 5 );
> + /* if it hasn't been done so far, stash a copy of the
> + * sequence header for muxers such as ogg */
> + /* The OggDirac spec advises that a Dirac EOS DataUnit
> + * is appended to the sequence header to allow guard
> + * against poor streaming servers */
> + /* XXX, should this be done using the packetizer ? */
> + p_enc->fmt_out.p_extra = malloc( len + sizeof(eos) );
> + if( !p_enc->fmt_out.p_extra )
> + {
> + p_enc->b_error = 1;
> + return NULL;
> + }
> + memcpy( p_enc->fmt_out.p_extra, p_block->p_buffer, len);
> + memcpy( (uint8_t*)p_enc->fmt_out.p_extra + len, eos, sizeof(eos) );
> + SetDWBE( (uint8_t*)p_enc->fmt_out.p_extra + len + 10, len );
> + p_enc->fmt_out.i_extra = len + sizeof(eos);
> + }
> }
> +
> + if( ReadDiracPictureNumber( &pic_num, p_block ) )
> {
> + /* Finding a picture terminates an ecapsulation unit, gather
> + * all data and output; use the next dts value queued up
> + * and find correct pts in the tlb */
> + p_block = block_FifoGet( p_sys->p_dts_fifo );
> + p_sys->p_chain->i_dts = p_block->i_dts;
> + p_sys->p_chain->i_pts = GetPicturePTS( p_enc, pic_num );
> + block_Release( p_block );
> + block_ChainAppend( &p_output_chain, block_ChainGather( p_sys->p_chain ) );
> + p_sys->p_chain = NULL;
> + } else {
> + p_block = NULL;
> + }
> + break;
> }
>
> - } while( state == ENC_STATE_AVAIL );
> - }
> - else
> - {
> - msg_Dbg( p_enc, "dirac_encoder_load() error" );
> - }
> + case ENC_STATE_BUFFER:
> + break;
> + case ENC_STATE_INVALID:
> + default:
> + break;
> + }
> + } while( state == ENC_STATE_AVAIL );
>
> - return p_chain;
> + return p_output_chain;
> }
>
> /*****************************************************************************
> @@ -239,12 +967,15 @@ static void CloseEncoder( vlc_object_t *p_this )
> encoder_t *p_enc = (encoder_t *)p_this;
> encoder_sys_t *p_sys = p_enc->p_sys;
>
> - msg_Dbg( p_enc, "resulting bit-rate: %lld bits/sec",
> - p_sys->p_dirac->enc_seqstats.bit_rate );
> -
> /* Free the encoder resources */
> - dirac_encoder_close( p_sys->p_dirac );
> -
> - free( p_sys->p_buffer_in );
> + if( p_sys->p_dirac )
> + dirac_encoder_close( p_sys->p_dirac );
> +
> + if( p_sys->p_buffer_in )
> + free( p_sys->p_buffer_in );
Useless test.
(not blocking)
> + block_FifoRelease( p_sys->p_dts_fifo );
Test against NULL needed.
--
fenrir
More information about the vlc-devel
mailing list