[FFmpeg-devel] [PATCH] Add ATRAC3+ decoder

[Michael Niedermayer]

On Sat, Oct 19, 2013 at 12:39:18AM +0200, Maxim Polijakowski wrote:
> Thank you for your suggestions!
> Please find an updated patch in my next email:
> [PATCH] ATRAC3+ decoder, 2nd try
> 
> Below a couple of comments...
[...]
> >>+        }
> >>+        code <<= 1;
> >>+    }
> >>+
> >>+    ff_init_vlc_sparse(out_vlc, max_len, index, bits, 1, 1, codes, 2, 2,
> >>+                       xlat, 1, 1, 0);
> >>+}
> >build_canonical_huff() could be done to all tables and the resulting
> >tables be put in the source. This would avoid the need for
> >build_canonical_huff() and simplify the code
> >also it would allow the tables to be shared between instances of a
> >loaded lib as they would be static const and not generated at runtime.
> >
> >making that change would likely increase the size of static const
> >tables by <50% though.
> >If you prefer to leave it as it is to avoid these larger tables
> >thats fine too
> 
> At the time being, I would prefer to leave the tables as is.
> Expanding these VLC tables is a long-term task and could be done
> later once the decoder has been merged.

ok with me

[...]
> >[...]
> >>+/** 3D base shape tables. The values are grouped together as follows:
> >>+ *  [num_start_values = 8][num_shape_tables = 16][num_seg_coeffs = 9]
> >>+ *  For each of the 8 start values there are 16 different shapes each
> >>+ *  9 coefficients long. */
> >>+const int8_t ff_atrac3p_wl_shapes[8][16][9] = {
> >some of the tables are only used from one file, these tables could
> >be static, reducing the global namespace polution and might speed up
> >linking in some cases on some platforms by a tiny amount
> 
> Most of these tables are only used from one file. Should I convert
> all of them to static?

yes


> Should I remove the prefix "ff_atrac3p_" as well?

ff_ should be removed if they are static
atrac3p_ could be removed too if you like or it could be kept ...


[...]
> >>+    switch (avctx->channel_layout) {
> >>+    case AV_CH_LAYOUT_MONO:
> >>+        ctx->num_channel_blocks = 1;
> >>+        ctx->channel_blocks[0]  = CH_UNIT_MONO;
> >>+        break;
> >>+    case AV_CH_LAYOUT_STEREO:
> >>+        ctx->num_channel_blocks = 1;
> >>+        ctx->channel_blocks[0]  = CH_UNIT_STEREO;
> >>+        break;
> >>+    case AV_CH_LAYOUT_SURROUND:
> >>+        ctx->num_channel_blocks = 2;
> >>+        ctx->channel_blocks[0]  = CH_UNIT_STEREO;
> >>+        ctx->channel_blocks[1]  = CH_UNIT_MONO;
> >>+        break;
> >>+    case AV_CH_LAYOUT_4POINT0:
> >>+        ctx->num_channel_blocks = 3;
> >>+        ctx->channel_blocks[0]  = CH_UNIT_STEREO;
> >>+        ctx->channel_blocks[1]  = CH_UNIT_MONO;
> >>+        ctx->channel_blocks[2]  = CH_UNIT_MONO;
> >>+        break;
> >>+    case AV_CH_LAYOUT_5POINT1_BACK:
> >>+        ctx->num_channel_blocks = 4;
> >>+        ctx->channel_blocks[0]  = CH_UNIT_STEREO;
> >>+        ctx->channel_blocks[1]  = CH_UNIT_MONO;
> >>+        ctx->channel_blocks[2]  = CH_UNIT_STEREO;
> >>+        ctx->channel_blocks[3]  = CH_UNIT_MONO;
> >>+        break;
> >>+    case AV_CH_LAYOUT_6POINT1_BACK:
> >>+        ctx->num_channel_blocks = 5;
> >>+        ctx->channel_blocks[0]  = CH_UNIT_STEREO;
> >>+        ctx->channel_blocks[1]  = CH_UNIT_MONO;
> >>+        ctx->channel_blocks[2]  = CH_UNIT_STEREO;
> >>+        ctx->channel_blocks[3]  = CH_UNIT_MONO;
> >>+        ctx->channel_blocks[4]  = CH_UNIT_MONO;
> >>+        break;
> >>+    case AV_CH_LAYOUT_7POINT1:
> >>+        ctx->num_channel_blocks = 5;
> >>+        ctx->channel_blocks[0]  = CH_UNIT_STEREO;
> >>+        ctx->channel_blocks[1]  = CH_UNIT_MONO;
> >>+        ctx->channel_blocks[2]  = CH_UNIT_STEREO;
> >>+        ctx->channel_blocks[3]  = CH_UNIT_STEREO;
> >>+        ctx->channel_blocks[4]  = CH_UNIT_MONO;
> >>+        break;
> >could be simplified
> >for example with something like this, if you like
> >
> >ctx->channel_blocks[0] = CH_UNIT_MONO;
> >if (channels > 1)
> >     ctx->channel_blocks[0] = CH_UNIT_STEREO;
> >if (channels > 2)
> >     ctx->channel_blocks[1] = CH_UNIT_MONO;
> >if (channels > 3)
> >     ctx->channel_blocks[2] = CH_UNIT_MONO;
> >if (channels > 4) {
> >     ctx->channel_blocks[2] = CH_UNIT_STEREO;
> >     ctx->channel_blocks[3] = CH_UNIT_MONO;
> >}
> >if (channels > 6)
> >     ctx->channel_blocks[4] = CH_UNIT_MONO;
> >if (channels > 7)
> >     ctx->channel_blocks[3] = CH_UNIT_STEREO;
> 
> It would just barely simplify the stuff because I need to estimate
> number of channel blocks as well.
> Moreover, it makes the code harder to read IMHO. Therefore, I'd
> prefer to leave it as is...

ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131019/7a429893/attachment.asc>