[vlc-devel] [PATCH] Support DVD LPCM encode

[Jean-Baptiste Kempf]

Hello,

On Sun, Sep 26, 2010 at 03:02:08PM +0200, Steinar H. Gunderson wrote :
> This patch adds encode support to the LPCM module; only the DVD variant for
> now. It's not been tested with any other clients than VLC.
Ok.

Missing #ifdef ENABLE_SOUT
> +struct encoder_sys_t
around this?

>   * LPCM DVD header :
> - * - frame number (8 bits)
> - * - unknown (16 bits) == 0x0003 ?
> - * - unknown (4 bits)
> - * - current frame (4 bits)
> - * - unknown (2 bits)
> - * - frequency (2 bits) 0 == 48 kHz, 1 == 32 kHz, 2 == ?, 3 == ?
> - * - unknown (1 bit)
> + * - number of frames in this packet (8 bits)
> + * - first access unit (16 bits) == 0x0003 ?
> + * - emphasis (1 bit)
> + * - mute (1 bit)
> + * - reserved (1 bit)
> + * - current frame (5 bits)
> + * - quantisation (2 bits) 0 == 16bps, 1 == 20bps, 2 == 24bps, 3 == illegal
> + * - frequency (2 bits) 0 == 48 kHz, 1 == 96 kHz, 2 == 44.1 kHz, 3 == 32 kHz
> + * - reserved (1 bit)
>   * - number of channels - 1 (3 bits) 1 == 2 channels
> - * - start code (8 bits) == 0x80
> + * - dynamic range (8 bits) 0x80 == neutral

Shouldn't this be in a separated patch?

> +static int OpenEncoder( vlc_object_t *p_this )
> +{
> +    encoder_t *p_enc = (encoder_t *)p_this;
> +    encoder_sys_t *p_sys;
> +
> +    /* We only support DVD LPCM yet. */
> +    if( p_enc->fmt_out.i_codec != VLC_CODEC_DVD_LPCM ||
> +        ( p_enc->fmt_in.audio.i_rate != 48000 &&
> +          p_enc->fmt_in.audio.i_rate != 96000 &&
> +          p_enc->fmt_in.audio.i_rate != 44100 &&
> +          p_enc->fmt_in.audio.i_rate != 32000 ) ||
> +       p_enc->fmt_in.audio.i_channels > 8 )
> +        return VLC_EGENERIC;
Maybe could you return EGENERIC if != VLC_CODEC_DVD_LPCM and then
if i_rate and i_channels don't match, make a message and return EGENERIC?

> +/*****************************************************************************
> + * EncodeFrames: encode zero or more LCPM audio packets
> + *****************************************************************************/
> +static block_t *EncodeFrames( encoder_t *p_enc, aout_buffer_t *p_aout_buf )
> +{
> +    switch( p_sys->i_rate ) {
> +    case 48000:
> +        i_freq_code = 0;
> +        break;
> +    case 96000:
> +        i_freq_code = 1;
> +        break;
> +    case 44100:
> +        i_freq_code = 2;
> +        break;
> +    case 32000:
> +        i_freq_code = 3;
> +        break;
> +    }
no default: assert(0) ?

Else: beware of trailing spaces in source code?

Best Regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/
+33 672 704 734