[vlc-devel] [PATCH 2/2] Opus in Transport Stream

[Rafaël Carré]

Hi,

On 09/10/14 03:27, Tristan Matthews wrote:
> Nice work! Comments inline
> 
> On Tue, Sep 9, 2014 at 11:46 AM, Rafaël Carré <funman at videolan.org> wrote:

>> +        if (!au_size || au_size > len) {
>> +            msg_Err(demux, "Invalid Opus AU size %d (PES %zu)", au_size, len);
>> +            goto end;
> 
> Why not just break?
> 
>> +        }
>> +
>> +        block_t *au = block_Alloc(au_size);
>> +        if (!au)
>> +            goto end;
> 
> Same.

Changed both to break and removed label.

>> +end:
>> +    block_Release(block);
>> +    return out;
>> +}
>> +

>> +static int vlc_ceil_log2( const unsigned int val )
>> +{
>> +    int n = 0;
>> +    static const int vlc_log2_table[16] =
>> +    {
>> +        0,0,1,1,2,2,2,2, 3,3,3,3,3,3,3,3
>> +    };
>> +
>> +    unsigned int v = val;
>> +
>> +    if( v&0xffff0000 )
>> +    {
>> +        v >>= 16;
>> +        n += 16;
>> +    }
>> +    if( v&0xff00 )
>> +    {
>> +        v >>= 8;
>> +        n += 8;
>> +    }
>> +    if( v&0xf0 )
>> +    {
>> +        v >>= 4;
>> +        n += 4;
>> +    }
>> +    n += vlc_log2_table[v];
>> +
>> +    // ceil
>> +
>> +    if ((1U << n) != val)
>> +        n++;
>> +
>> +    return n;
>> +}

I will remove that function and use something simple as it only runs
at PMT setup.

>> +explicit_config_too_short:
>> +    msg_Err(demux, "too short Opus descriptor");
> 
> nit: "Opus descriptor too short" might be better.

OK

>> +    case VLC_CODEC_OPUS:
>> +        if (p_input->p_fmt->audio.i_channels > 8) {
>> +            msg_Err(p_mux, "Too much opus channels (%d > 8)",
> 
> nit: "Too many Opus channels"

More than 8 might actually be uncountable, but ok ;)

>> --
> 
> I created and tested some mono, stereo and 8 channel material without
> any issues.

Nice :)

Thanks for the review

> Best,
> Tristan