[vlc-devel] Opus RTP raw audio decoder plugin patch
Yupeng Chang
changyp6 at gmail.com
Thu Nov 28 11:35:01 CET 2013
Hi Ilkka,
Thanks very much for your reviewing.
There are some details I didn't noticed, because this patch is modified
based on opus.c
In fact, I can just modify opus.c and add Opus Raw decoding into
DecodeBlock function in opus.c,
but this requires OPUS RTP and OggOpus share the same Fourcc, which is
VLC_CODEC_OPUS
I don't know which method would you prefer.
OggOpus and Opus Raw have different APIs to do decoding.
And current Opus decoder only accepts Ogg Packet which contains Opus data,
but Opus data depacketized from RTP payload is Opus Raw, which can be
decoded directly.
Either implement OggOpus and RawOpus decoding in the same opus decoder
plugin or implement them independently.
Looking forward to your reply!
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/2ec76a20/attachment.html>
More information about the vlc-devel
mailing list