[FFmpeg-devel] [PATCH 1/2] Decode DTS XXCH extension

[Nick Brereton]

> This will mostly be a cosmetics review, since I'm rather unqualified
> to review DCA-specific things.
>
[...]
>
>> +    /* XXCH extension information */
>> +    int xxch_spk_layout;
>> +    int xxch_nbits_spkr_mask;
>
> I'm assuming spk == spkr == speaker. Should probably be consistent.

Made everything spk.

[...]

>
>> +        if(get_bits1(&s->gb)) {
>> +            unsigned dmix_embedded = get_bits1(&s->gb);
>> +            unsigned scale_factor = get_bits(&s->gb, 6);
>> +            unsigned mask[8];
>
> As far as I am aware, FFmpeg prefers to declare all variables at the
> beginning of the function. Also, it would aid in the upcoming MSVC
> support effort.
>
> Any particular reason these need to be unsigned? Especially the one
> that is only 1 bit.

Only that they can never be negative, and that some compilers will 
produce better code with the extra constraint. However I've changed 
these to ints.

[...]
>
>> -#ifdef TRACE
>> +#if defined(TRACE)
>
> Why?

Edited to find out what values I was getting -- shouldn't have got into 
patch, I've put it back.

[...]
>
>> +    /* HACK: get the XCH layout to check my 6.1 sample */
>> +    av_log(s->avctx, AV_LOG_INFO, "XXCH extension : fudged channel
>> layout!\n");
>> +    s->xch_present = 1;
>
> I don't know if this should really be present in the final patch...

That wasn't supposed to have escaped from the zoo. Removed.

>
>> +        else if(mkr == 0x47004a03)
>> +            av_log(s->avctx, AV_LOG_DEBUG, "DTS-MA: got XXCH in 
>> MA\n");
>
> I'm not exactly sure what this means. Perhaps it can be reworded?

Removed -- not needed.

>
> I didn't see anything obviously wrong from a technical standpoint,
> but I'm hardly an expert.
>
> - Derek
>

Attached is an updated version of the patch which also takes care of 
the formatting nits you had, moves all variable declarations to the top 
of functions and expands the comment about XXCH headers differing 
slightly from XCH and core subframe headers.

I'll resend the second patch to take account of this update & clean up 
formatting.

Nick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Parse-decode-DTS-XXCH-frames.patch
Type: text/x-patch
Size: 7200 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120625/18397cf2/attachment.bin>