[vlc-devel] [PATCH 2/3] Enable VP8 RTP packetization

[Andrey Utkin]

2013/12/8 Jean-Baptiste Kempf <jb at videolan.org>:
> On 08 Dec, Andrey Utkin wrote :
>> +#define RTP_VP8_HEADER_SIZE 1
>> +#define RTP_VP8_PAYLOAD_START (12 + RTP_VP8_HEADER_SIZE)
>
> Extra line break please.

Edited.

>> +static int rtp_packetize_vp8( sout_stream_id_t *id, block_t *in )
>> +{
>> +    int     i_max   = rtp_mtu (id) - RTP_VP8_HEADER_SIZE;
>> +    int     i_count = ( in->i_buffer + i_max - 1 ) / i_max;
>> +
>> +    uint8_t *p_data = in->p_buffer;
>> +    int     i_data  = in->i_buffer;
>> +    int     i;
>> +
>> +    for( i = 0; i < i_count; i++ )
>
> Please, use C99

Moved i declaration into for ( ).

>> +    {
>> +        int i_payload = __MIN( i_max, i_data );
>
> Is that guaranteed to be positive?

If we can rely that in->i_buffer is positive, then to guarantee
i_payload > 0 we have to ensure that i_max > 0.
Would such check be OK?
if (i_max <= 0)
    return VLC_EGENERIC;

BTW This should then be added everywhere amongst rtp_packetize_* functions.

>> +        block_t *out = block_Alloc( RTP_VP8_PAYLOAD_START + i_payload );
>
> block_Alloc cannot fail?

It is not checked in nearly all rtp_packetize_* functions.
The only function that checks result of block_Alloc() is
rtp_packetize_t140, and it returns VLC_SUCCESS in case of fail, i
doubt this is correct and maybe there should be VLC_ENOMEM return.
Please comment.
Edited with check and return of VLC_ENOMEM.


Took your points into consideration for JPEG patch.

-- 
Andrey Utkin