[vlc-devel] Opus RTP raw audio decoder plugin patch
Ilkka Ollakka
ileoo at videolan.org
Thu Nov 28 09:17:52 CET 2013
On Wed, Nov 27, 2013 at 08:12:57PM +0800, Yupeng Chang wrote:
> Hi All,
Hi,
> Please kindly review and give me some feed backs!
I would prefer that we would just just one decoder and packetizers. But
few remarks inline.
> index 824cdf2..25e64f5 100644
> --- a/modules/access/live555.cpp
> +++ b/modules/access/live555.cpp
> @@ -980,6 +980,17 @@ static int SessionsSetup( demux_t *p_demux )
> else
> msg_Warn( p_demux,"Missing or unsupported vorbis header." );
> }
> + else if( !strcmp(sub->codecName(), "OPUS") )
> + {
> + tk->fmt.i_codec = VLC_CODEC_OPUS_RTP;
> + if( sub->rtpTimestampFrequency() )
> + tk->fmt.audio.i_rate = sub->rtpTimestampFrequency();
> + else
> + {
> + msg_Warn(p_demux, "Using 48kHz as default sample rate.");
> + tk->fmt.audio.i_rate = 48000;
> + }
> + }
> }
> else if( !strcmp( sub->mediumName(), "video" ) )
> {
This could be separated patch as it enables opus from rtp
> diff --git a/modules/codec/opus_rtp.c b/modules/codec/opus_rtp.c
> new file mode 100644
> index 0000000..c7046c1
> + *****************************************************************************
> + * Copyright (C) 2003-2009, 2012 VLC authors and VideoLAN
Years are not right here are they?
> +#include "../demux/xiph.h"
This could use comment why it is included
> +static const int pi_channels_maps[9] =
> +{
> + 0,
> + AOUT_CHAN_CENTER,
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT,
> + AOUT_CHAN_CENTER | AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT,
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_REARLEFT
> + | AOUT_CHAN_REARRIGHT,
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_CENTER
> + | AOUT_CHAN_REARLEFT | AOUT_CHAN_REARRIGHT,
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_CENTER
> + | AOUT_CHAN_REARLEFT | AOUT_CHAN_REARRIGHT | AOUT_CHAN_LFE,
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_CENTER
> + | AOUT_CHAN_REARCENTER | AOUT_CHAN_MIDDLELEFT
> + | AOUT_CHAN_MIDDLERIGHT | AOUT_CHAN_LFE,
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_CENTER | AOUT_CHAN_REARLEFT
> + | AOUT_CHAN_REARRIGHT | AOUT_CHAN_MIDDLELEFT | AOUT_CHAN_MIDDLERIGHT
> + | AOUT_CHAN_LFE,
> +};
This isn't anything similar to vlc normal channel maps already defined?
> +/****************************************************************************
> + * Local prototypes
> + ****************************************************************************/
> +static block_t *DecodeBlock ( decoder_t *, block_t ** );
Rearrange file so this isn't needed.
> +/*****************************************************************************
> + * OpenDecoder: probe the decoder and return score
> + *****************************************************************************/
> +static int OpenDecoder( vlc_object_t *p_this )
> +{
> + decoder_t *p_dec = (decoder_t*)p_this;
> + decoder_sys_t *p_sys;
> +
> + if( p_dec->fmt_in.i_codec != VLC_CODEC_OPUS_RTP )
> + return VLC_EGENERIC;
> +
> + /* Allocate the memory needed to store the decoder's structure */
> + if( ( p_dec->p_sys = p_sys = malloc(sizeof(decoder_sys_t)) ) == NULL )
> + return VLC_ENOMEM;
> +
> + date_Set( &p_sys->end_date, 0 );
You set date here as 0, but later you initialize it always with
48k, couldn't you initialize the date here already with 48k as it's set
to 0 at initialise.
> +/****************************************************************************
> + * DecodeBlock: the whole thing
> + ****************************************************************************
> + * This function must be fed with ogg packets.
Not sure this comment is valid
> +static block_t *DecodeBlock( decoder_t *p_dec, block_t **pp_block )
> +{
> + block_t *p_aout_buffer = NULL;
> + block_t *p_block = *pp_block;
> + unsigned char *data = NULL;
> + int buf_size = 32768;
Does this buf_size come from somewhere? needs a comment
> + struct timeval pts;
Where do you use this?
> + /* Date management */
> + if( p_block && p_block->i_pts > VLC_TS_INVALID &&
> + p_block->i_pts != date_Get( &p_sys->end_date ) )
> + {
> + date_Set( &p_sys->end_date, p_block->i_pts );
> + }
I don't think you need to use that end_date, as you just set output pts
same as input pts?
> + if( !date_Get( &p_sys->end_date ) )
> + {
> + /* We've just started the stream, wait for the first PTS. */
> + if( p_block ) block_Release( p_block );
> + return NULL;
> + }
> +
> + *pp_block = NULL; /* To avoid being fed the same packet again */
> +
> + p_aout_buffer = decoder_NewAudioBuffer(p_dec, buf_size);
> + if ( !p_aout_buffer )
> + {
> + msg_Err(p_dec, "Oops: No new buffer was returned!");
> + return NULL;
> + }
Maybe you should release the block before return
> + /*
> + unsigned char toc;
> + unsigned char fc;
> + unsigned char frames[48];
> + opus_int16 sizes[48];
> + int nb_frames;
> + int bytes_read;
> + int payload_offset;
> + nb_frames = opus_packet_parse(data, size, &toc, (const unsigned char**)&frames, (opus_int16*)&sizes, &payload_offset);
> + fc = data[1];
> + written = 0; bytes_read = 0;
> + printf ("toc %02x (%d %d %d) - frame count %02x (%d %d %d) - offset %d\n",
> + toc, (toc>>3) & 0x1F, (toc>>4) & 0x01, toc & 0x03, fc, (fc>>7) & 0x01, (fc>>6) & 0x01, fc & 0x3F, payload_offset);
> + for (int i=0; i<nb_frames; ++i)
> + {
> + printf ("frame %d - size %d\n", i, sizes[i]);
> + data[bytes_read+1] = toc & 0xFC;
> + written += opus_decode(dec, data + 1 + bytes_read, sizes[i] + 1, p_aout_buffer->p_buffer + written, 4096, 0) * 4; // data
> + bytes_read += sizes[i];
> + }
> + */
Unused code so why it is there?
> + written = opus_decode(p_sys->p_raw_dec,
> + data, size,
> + (opus_int16*)p_aout_buffer->p_buffer,
> + buf_size / nb_channels, 0) * nb_channels * 2; // data
> +
> + p_aout_buffer->i_nb_samples = nb_samples;
> + p_aout_buffer->i_pts = date_Get( &p_sys->end_date );
if input doesn't have pts (VLC_TS_INVALID) it's ok to set pts to be the
previous pts value?
> + if( p_block ) block_Release( p_block );
> +
> + return p_aout_buffer;
--
Ilkka Ollakka
Platonic Shadow:
A nonsexual friendship with a member of the opposite sex.
-- Douglas Coupland, "Generation X: Tales for an Accelerated
Culture"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20131128/6bd75199/attachment.sig>
More information about the vlc-devel
mailing list