[vlc-devel] Opus RTP raw audio decoder plugin patch
Yupeng Chang
changyp6 at gmail.com
Thu Nov 28 13:24:11 CET 2013
Hi Ilkka Ollakka,
I have modified the patch according to your comments.
I added one function ProcessRawData in opus.c to do Opus raw decoding.
Patches are attached.
Could you help review again please ?
Thanks!
Yupeng Chang
Nov-28-2013
On 28 November 2013 16:17, Ilkka Ollakka <ileoo at videolan.org> wrote:
> 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"
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20131128/e215e542/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-access-Enable-Opus-RTP.patch
Type: text/x-patch
Size: 1225 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20131128/e215e542/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-codec-Add-Opus-raw-decoding.patch
Type: text/x-patch
Size: 7597 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20131128/e215e542/attachment-0001.bin>
More information about the vlc-devel
mailing list