<div dir="ltr"><div><div><div><div><div><div>Hi <span name="Ilkka Ollakka" class="">Ilkka,<br></span></div><span name="Ilkka Ollakka" class="">Thanks very much for your reviewing.<br></span></div><span name="Ilkka Ollakka" class=""><br>
</span></div><span name="Ilkka Ollakka" class="">There are some details I didn't noticed, because this patch is modified based on opus.c<br><br></span></div><span name="Ilkka Ollakka" class="">In fact, I can just modify opus.c and add Opus Raw decoding into DecodeBlock function in opus.c,<br>
</span></div><span name="Ilkka Ollakka" class="">but this requires OPUS RTP and OggOpus share the same Fourcc, which is VLC_CODEC_OPUS<br><br></span></div><div><span name="Ilkka Ollakka" class="">I don't know which method would you prefer.<br>
<br></span></div><div><span name="Ilkka Ollakka" class="">OggOpus and Opus Raw have different APIs to do decoding.<br></span></div><div><span name="Ilkka Ollakka" class="">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.<br>
<br></span></div><div><span name="Ilkka Ollakka" class="">Either implement OggOpus and RawOpus decoding in the same opus decoder plugin or implement them independently.<br><br></span></div><div><span name="Ilkka Ollakka" class="">Looking forward to your reply!<br>
<br></span></div><div><span name="Ilkka Ollakka" class="">Thanks !<br><br></span></div><div><span name="Ilkka Ollakka" class="">Yupeng Chang<br></span></div><div><span name="Ilkka Ollakka" class="">Nov-28, 2013<br></span></div>
<span name="Ilkka Ollakka" class=""></span></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 28 November 2013 16:17, Ilkka Ollakka <span dir="ltr"><<a href="mailto:ileoo@videolan.org" target="_blank">ileoo@videolan.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, Nov 27, 2013 at 08:12:57PM +0800, Yupeng Chang wrote:<br>
> Hi All,<br>
<br>
Hi,<br>
<div class="im"><br>
> Please kindly review and give me some feed backs!<br>
<br>
</div>I would prefer that we would just just one decoder and packetizers. But<br>
few remarks inline.<br>
<br>
> index 824cdf2..25e64f5 100644<br>
> --- a/modules/access/live555.cpp<br>
> +++ b/modules/access/live555.cpp<br>
> @@ -980,6 +980,17 @@ static int SessionsSetup( demux_t *p_demux )<br>
> else<br>
> msg_Warn( p_demux,"Missing or unsupported vorbis header." );<br>
> }<br>
> + else if( !strcmp(sub->codecName(), "OPUS") )<br>
> + {<br>
> + tk->fmt.i_codec = VLC_CODEC_OPUS_RTP;<br>
> + if( sub->rtpTimestampFrequency() )<br>
> + tk->fmt.audio.i_rate = sub->rtpTimestampFrequency();<br>
> + else<br>
> + {<br>
> + msg_Warn(p_demux, "Using 48kHz as default sample rate.");<br>
> + tk->fmt.audio.i_rate = 48000;<br>
> + }<br>
> + }<br>
> }<br>
> else if( !strcmp( sub->mediumName(), "video" ) )<br>
> {<br>
<br>
This could be separated patch as it enables opus from rtp<br>
<br>
> diff --git a/modules/codec/opus_rtp.c b/modules/codec/opus_rtp.c<br>
> new file mode 100644<br>
> index 0000000..c7046c1<br>
> + *****************************************************************************<br>
> + * Copyright (C) 2003-2009, 2012 VLC authors and VideoLAN<br>
<br>
Years are not right here are they?<br>
<br>
> +#include "../demux/xiph.h"<br>
<br>
This could use comment why it is included<br>
<br>
> +static const int pi_channels_maps[9] =<br>
> +{<br>
> + 0,<br>
> + AOUT_CHAN_CENTER,<br>
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT,<br>
> + AOUT_CHAN_CENTER | AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT,<br>
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_REARLEFT<br>
> + | AOUT_CHAN_REARRIGHT,<br>
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_CENTER<br>
> + | AOUT_CHAN_REARLEFT | AOUT_CHAN_REARRIGHT,<br>
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_CENTER<br>
> + | AOUT_CHAN_REARLEFT | AOUT_CHAN_REARRIGHT | AOUT_CHAN_LFE,<br>
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_CENTER<br>
> + | AOUT_CHAN_REARCENTER | AOUT_CHAN_MIDDLELEFT<br>
> + | AOUT_CHAN_MIDDLERIGHT | AOUT_CHAN_LFE,<br>
> + AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT | AOUT_CHAN_CENTER | AOUT_CHAN_REARLEFT<br>
> + | AOUT_CHAN_REARRIGHT | AOUT_CHAN_MIDDLELEFT | AOUT_CHAN_MIDDLERIGHT<br>
> + | AOUT_CHAN_LFE,<br>
> +};<br>
<br>
This isn't anything similar to vlc normal channel maps already defined?<br>
<br>
> +/****************************************************************************<br>
> + * Local prototypes<br>
> + ****************************************************************************/<br>
> +static block_t *DecodeBlock ( decoder_t *, block_t ** );<br>
<br>
Rearrange file so this isn't needed.<br>
<br>
> +/*****************************************************************************<br>
> + * OpenDecoder: probe the decoder and return score<br>
> + *****************************************************************************/<br>
> +static int OpenDecoder( vlc_object_t *p_this )<br>
> +{<br>
> + decoder_t *p_dec = (decoder_t*)p_this;<br>
> + decoder_sys_t *p_sys;<br>
> +<br>
> + if( p_dec->fmt_in.i_codec != VLC_CODEC_OPUS_RTP )<br>
> + return VLC_EGENERIC;<br>
> +<br>
> + /* Allocate the memory needed to store the decoder's structure */<br>
> + if( ( p_dec->p_sys = p_sys = malloc(sizeof(decoder_sys_t)) ) == NULL )<br>
> + return VLC_ENOMEM;<br>
> +<br>
> + date_Set( &p_sys->end_date, 0 );<br>
<br>
You set date here as 0, but later you initialize it always with<br>
48k, couldn't you initialize the date here already with 48k as it's set<br>
to 0 at initialise.<br>
<br>
> +/****************************************************************************<br>
> + * DecodeBlock: the whole thing<br>
> + ****************************************************************************<br>
> + * This function must be fed with ogg packets.<br>
<br>
Not sure this comment is valid<br>
<br>
> +static block_t *DecodeBlock( decoder_t *p_dec, block_t **pp_block )<br>
> +{<br>
> + block_t *p_aout_buffer = NULL;<br>
> + block_t *p_block = *pp_block;<br>
> + unsigned char *data = NULL;<br>
> + int buf_size = 32768;<br>
<br>
Does this buf_size come from somewhere? needs a comment<br>
<br>
> + struct timeval pts;<br>
<br>
Where do you use this?<br>
<br>
> + /* Date management */<br>
> + if( p_block && p_block->i_pts > VLC_TS_INVALID &&<br>
> + p_block->i_pts != date_Get( &p_sys->end_date ) )<br>
> + {<br>
> + date_Set( &p_sys->end_date, p_block->i_pts );<br>
> + }<br>
<br>
I don't think you need to use that end_date, as you just set output pts<br>
same as input pts?<br>
<br>
> + if( !date_Get( &p_sys->end_date ) )<br>
> + {<br>
> + /* We've just started the stream, wait for the first PTS. */<br>
> + if( p_block ) block_Release( p_block );<br>
> + return NULL;<br>
> + }<br>
> +<br>
> + *pp_block = NULL; /* To avoid being fed the same packet again */<br>
> +<br>
> + p_aout_buffer = decoder_NewAudioBuffer(p_dec, buf_size);<br>
> + if ( !p_aout_buffer )<br>
> + {<br>
> + msg_Err(p_dec, "Oops: No new buffer was returned!");<br>
> + return NULL;<br>
> + }<br>
<br>
Maybe you should release the block before return<br>
<br>
<br>
> + /*<br>
> + unsigned char toc;<br>
> + unsigned char fc;<br>
> + unsigned char frames[48];<br>
> + opus_int16 sizes[48];<br>
> + int nb_frames;<br>
> + int bytes_read;<br>
> + int payload_offset;<br>
> + nb_frames = opus_packet_parse(data, size, &toc, (const unsigned char**)&frames, (opus_int16*)&sizes, &payload_offset);<br>
> + fc = data[1];<br>
> + written = 0; bytes_read = 0;<br>
> + printf ("toc %02x (%d %d %d) - frame count %02x (%d %d %d) - offset %d\n",<br>
> + toc, (toc>>3) & 0x1F, (toc>>4) & 0x01, toc & 0x03, fc, (fc>>7) & 0x01, (fc>>6) & 0x01, fc & 0x3F, payload_offset);<br>
> + for (int i=0; i<nb_frames; ++i)<br>
> + {<br>
> + printf ("frame %d - size %d\n", i, sizes[i]);<br>
> + data[bytes_read+1] = toc & 0xFC;<br>
> + written += opus_decode(dec, data + 1 + bytes_read, sizes[i] + 1, p_aout_buffer->p_buffer + written, 4096, 0) * 4; // data<br>
> + bytes_read += sizes[i];<br>
> + }<br>
> + */<br>
<br>
Unused code so why it is there?<br>
<br>
> + written = opus_decode(p_sys->p_raw_dec,<br>
> + data, size,<br>
> + (opus_int16*)p_aout_buffer->p_buffer,<br>
> + buf_size / nb_channels, 0) * nb_channels * 2; // data<br>
> +<br>
> + p_aout_buffer->i_nb_samples = nb_samples;<br>
> + p_aout_buffer->i_pts = date_Get( &p_sys->end_date );<br>
<br>
if input doesn't have pts (VLC_TS_INVALID) it's ok to set pts to be the<br>
previous pts value?<br>
<br>
> + if( p_block ) block_Release( p_block );<br>
> +<br>
> + return p_aout_buffer;<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Ilkka Ollakka<br>
Platonic Shadow:<br>
A nonsexual friendship with a member of the opposite sex.<br>
-- Douglas Coupland, "Generation X: Tales for an Accelerated<br>
Culture"<br>
</font></span><br>_______________________________________________<br>
vlc-devel mailing list<br>
To unsubscribe or modify your subscription options:<br>
<a href="https://mailman.videolan.org/listinfo/vlc-devel" target="_blank">https://mailman.videolan.org/listinfo/vlc-devel</a><br>
<br></blockquote></div><br></div>