[FFmpeg-devel] theora depayloader

[Josh Allmann]

Updated patch attached, with fixes that everybody suggested. Comments below:

On 19 March 2010 12:50, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Fri, Mar 19, 2010 at 4:40 AM, Josh Allmann <joshua.allmann at gmail.com> wrote:
>> Here is the theora depayloader for my gsoc small task.
>> (http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2010-March/084179.html)
>
> Thanks for the patch! Review follows:
>
>> Index: libavformat/rtsp.c
> [..]
>> ? ? ? ? ?break;
>> + ? ?case CODEC_ID_THEORA:
>> + ? ? ? ?ff_theora_parse_fmtp_config(codec, ctx, attr, value);
>> ? ? ?case CODEC_ID_VORBIS:
>
> Missing break.

Done.

>
> Index: libavformat/Makefile
> [..]
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?rtpdec_h264.o \
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?rtpdec_vorbis.o
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?rtpdec_vorbis.o \
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?rtpdec_theora.o
>> ?OBJS-$(CONFIG_SEGAFILM_DEMUXER) ? ? ? ? ?+= segafilm.o
>
> Please keep this ordered alphabetically.

Done.

>
>> +struct PayloadContext {
>> + ? ?unsigned ident; ? ? ? ? ? ? ///< 24-bit stream configuration identifier
>> + ? ?int allocated_size; ? ?///< size allocated to buffer
>> + ? ?int fragment_length; ? ///< how much of the buffer actually used
>> + ? ?uint8_t* theora_fragment; ? ///< buffer for split payloads
>> +};
>
> Please vertically align the comments.

Done.

>
>> +static PayloadContext *theora_new_context(void)
>> +{
>> + ? ?PayloadContext* data = av_mallocz(sizeof(PayloadContext));
>> + ? ?data->allocated_size = 2046; // larger than typical MTU
>> + ? ?data->theora_fragment = av_malloc(data->allocated_size);
>> + ? ?return data;
>> +}
>
> You can vertically align this also, so it loose prettier:
>
> x ? = bla;
> xyz = blabla;

Done.

>
>> +static inline void check_size(PayloadContext * data,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int pkt_len)
>> +{
>> + ? ?// check for overflows, enlarge buffer if needed
>> + ? ?if (data->allocated_size < data->fragment_length+pkt_len){
>> + ? ? ? ?data->allocated_size *= 2;
>> + ? ? ? ?data->theora_fragment = av_realloc(data->theora_fragment, data->allocated_size);
>> + ? ?}
>> +}
>
> Do you really need a separate function for this? Also, I'd like you to
> consider using a dynamic packet buffer here, see url_open_dyn_buf()
> and related functions, ASF/RTP uses this also.

Done.

>
>> + ? ?// read theora rtp headers
>> + ? ?ident = AV_RB24(buf);
>> + ? ?fragmented = buf[3] >> 6;
>> + ? ?tdt = (buf[3] >> 4) & 3;
>> + ? ?num_pkts = buf[3] & 7;
>> + ? ?pkt_len = AV_RB16(buf + 4);
>
> Vertically align.

Done.

>
>> +static int get_base128(const uint8_t ** buf, const uint8_t * buf_end)
>> +{
>> + ? ?int n = 0;
>> + ? ?for (; *buf < buf_end; ++*buf) {
>> + ? ? ? ?n <<= 7;
>> + ? ? ? ?n += **buf & 0x7f;
>> + ? ? ? ?if (!(**buf & 0x80)) {
>> + ? ? ? ? ? ?++*buf;
>> + ? ? ? ? ? ?return n;
>> + ? ? ? ?}
>> + ? ?}
>> + ? ?return 0;
>> +}
>
> Do we have functions for this in libavutil?

It is common to vorbis, but I'm not sure if the encoding is
xiph-specific; the rfc description makes no mention of base128, but i
didn't read it that closely. Will refactor out this weekend.

>
>> +int ff_theora_parse_fmtp_config(AVCodecContext * codec,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *theora_data, char *attr, char *value)
> [..]
>
> Your indenting here is way off...

Fixed (i think?)

Thanks for the review! Updated patch attached.

Josh

>
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: theora_depayloader.diff
Type: text/x-patch
Size: 14936 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100319/798c7284/attachment.bin>