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

[Derek Buitenhuis]

This will mostly be a cosmetics review, since I'm rather unqualified
to review DCA-specific things.

On 24/06/2012 2:36 PM, Nick Brereton wrote:
> The attached patch will decode the DTS XXCH extension giving > 5.1 
> channels for DTS-HRA streams. The second patch handling signalling the 
> extra channels and their positions.
> 
> Note that most 7.1 DTS streams use the XLL (lossless) extension to 
> carry the channels, rather than XXCH, so will generally only decode as a 
> 5.1 core stream.
> 
> I also noticed that in the ETSI specification there is a bit called 
> FixedBit in the core frame header which is said to always be zero; the 
> current decoder takes that bit to indicate the presense of stereo 
> downmix coefficients (sets s->downmix) -- presumably DTS changed their 
> mind at some-stage as to the meaning of this bit; but if they've decided 
> to specify it as always zero does this mean that the option was never 
> used? In which case the dca decoder could be cleaned up by removing 
> handling for the stereo downmix coefficients?

[...]

> +    /* XXCH extension information */
> +    int xxch_spk_layout;
> +    int xxch_nbits_spkr_mask;

I'm assuming spk == spkr == speaker. Should probably be consistent.

> -static int dca_parse_audio_coding_header(DCAContext *s, int base_channel)
> +static int dca_parse_audio_coding_header(DCAContext *s, int base_channel, int xxch)

NIT: This brings it to >80 cols. Should probably be wrapped.

>  {
>      int i, j;
>      static const float adj_table[4] = { 1.0, 1.1250, 1.2500, 1.4375 };
>      static const int bitlen[11] = { 0, 1, 2, 2, 2, 2, 3, 3, 3, 3, 3 };
>      static const int thr[11]    = { 0, 1, 3, 3, 3, 3, 7, 7, 7, 7, 7 };
> +    int hdr_pos=0, hdr_size=0;

Please add around the equals signs.

> +    /* xxch has arbitrary sized audio coding headers */
> +    if(xxch) {

FFmpeg uses a space after the if:

if (stuff) {

I won't note the rest of the occurrences, as it'd just fill
the review with redundancy.

> +        hdr_pos = get_bits_count(&s->gb);
> +        hdr_size = get_bits(&s->gb, 7) + 1;

NIT: Align.

> +        i = get_bits(&s->gb, s->xxch_nbits_spkr_mask - 6);

NIT: In general, reusing the i variable for something here seems
to be somewhat undescriptive and confusing.

> +        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.

> +            for(i = base_channel; i < s->prim_channels; i++) {

Same rule with for statements as with if statements.

> +        /* Skip to the end of the header, also ignore CRC if present  */
> +        i = get_bits_count(&s->gb);

Same as before.

> -#ifdef TRACE
> +#if defined(TRACE)

Why?

> +/* parse intial header for XXCH and dump details */

Typo.

> +    unsigned hdr_size, chhdr_crc, spkmsk_bits, num_chsets, core_speakers, hdr_pos;
> +    unsigned i, chset, base_channel, fsize[8];

NIT: Why unsigned?
NIT: 80 cols.

> +    for(i = 0; i < num_chsets; ++i)
> +        fsize[i] = get_bits(&s->gb, 14) + 1;

Every other for loop uses i++.

> +    av_log(s->avctx, AV_LOG_DEBUG, "Core Speaker Mask : 0x%08x\n", core_speakers);

NIT: 80 cols.

> +    s->xxch_spk_layout = core_speakers;
> +    s->xxch_nbits_spkr_mask = spkmsk_bits;

NIT: Align.

> +    /* skip to the end of the header */
> +    i = get_bits_count(&s->gb);

Same as before.

> +    /* loop though the channel sets & decode additional channels as we go along */
> +    for(chset = 0; chset < num_chsets; chset++) {
> +        unsigned chstart = get_bits_count(&s->gb);

Same note about mid-function variable declarations as before.
NIT: 80 cols.

> +        /* decode the header, it's slightly different for XXCH than core & XCH frames */

In what way? 
NIT: 80 cols.

Not gonna note any more 80 col nits, but they're assumed.

> +    /* 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...

> +        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?

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

- Derek

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 898 bytes
Desc: OpenPGP digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120624/d388248a/attachment.asc>