[FFmpeg-devel] [PATCH] Remove only use of compound literals in FFmpeg.

[Reimar Döffinger]

On Mon, Dec 30, 2013 at 11:22:06AM +0100, Clément Bœsch wrote:
> On Mon, Dec 30, 2013 at 11:07:42AM +0100, Reimar Döffinger wrote:
> > On Mon, Dec 30, 2013 at 11:00:52AM +0100, Reimar Döffinger wrote:
> > > On Mon, Dec 30, 2013 at 09:00:13AM +0100, Clément Bœsch wrote:
> > > > On Mon, Dec 30, 2013 at 08:28:13AM +0100, Reimar Döffinger wrote:
> > > > > On 30.12.2013, at 07:31, Clément Bœsch <u at pkh.me> wrote:
> > > > > > On Mon, Dec 30, 2013 at 01:09:17AM +0100, Nicolas George wrote:
> > > > > >> Le decadi 10 nivôse, an CCXXII, Reimar Döffinger a écrit :
> > > > > >>> However it still seems to be the only place where we use it?
> > > > > >> 
> > > > > >> I am afraid not: there is also the av_err2str() magic macro (in
> > > > > >> lavu/error.h), which is, if I say so myself, really convenient to use
> > > > > >> instead of the normal functions.
> > > > > >> 
> > > > > >> I suppose you could implement it using a static array, at the cost of 64
> > > > > >> extra bytes per use.
> > > > > 
> > > > > It can always be implemented as a local array with no disadvantage, it never does more than avoiding a local variable.
> > > > 
> > > > having a char buf[INCONSISTENT_SIZE] on top of a large function for an
> > > > label error at the end is a bit troublesome. Of course you can do it, but
> > > >     fprintf(stderr, "Error [%s]\n", av_err2str(ret));
> > > > is more cute than
> > > >     char buf[128];
> > > >     fprintf(stderr, "Error [%s]\n", av_make_error_string(buf, sizeof(buf), ret));
> > > 
> > > Sorry, I missed it was a function, not a macro.
> > > 
> > > > > In many cases it does less, since many compilers are unable to place them in .rodata and always build them on stack.
> > > > > 
> > > > > > Same goes for av_ts2str().
> > > > > 
> > > > > Is that function unused? I didn't notice it causing any issue.
> > > > 
> > > > Yes, in many place, and often in the same function call. To change that
> > > > you would need to declare like 4 or more "char bufN[64]", which sounds
> > > > like an expensive price for supporting a compiler no one uses anymore.
> > > 
> > > Oh, no doubt about that (though "no one" isn't _quite_ true, for better
> > > or worse). While I haven't tested if it actually works, that code however
> > > compiles just fine...
> > > So there is something special about that aresample code.
> > > Note that one of the reasons I investigate it is because I am always
> > > _very_ suspicious of any language feature that is only used in one
> > > single file in a whole project. Most of the time IMHO that means either
> > > it should be used in other places as well, or it's not a good idea to be
> > > used in that one file either.
> > > However at this point I seem to have made the wrong conclusions on what
> > > that feature actually is...
> > 
> > Ok, it seems that gcc 2.95 treats these like static initializers, all
> > elements of a compound literal need to be constant.
> > That is why all other cases work, this one is the only one with
> > non-constant initialization values.
> 
> > That leaves the question if you think this patch is reasonable.
> 
> I don't mind the af_aresample.c change (but I'm not a maintainer of that
> filter), I don't think it hurts, I was just a bit reluctant about the
> changes with av_{err,ts}2str()
> 
> Thought, it's strange that it only complains in that place:
> 
> [~/src/ffmpeg]☭ git grep '(int\[\])'
> libavcodec/ac3enc.c:            num_blocks = ((int[]){ 1, 2, 3, 6 })[num_blks_code];
> libavcodec/flacenc.c:    s->options.block_time_ms = ((int[]){ 27, 27, 27,105,105,105,105,105,105,105,105,105,105
> libavcodec/flacenc.c:        s->options.lpc_type  = ((int[]){ FF_LPC_TYPE_FIXED,    FF_LPC_TYPE_FIXED,    FF_LPC
> libavcodec/flacenc.c:    s->options.min_prediction_order = ((int[]){  2,  0,  0,  1,  1,  1,  1,  1,  1,  1,  1,
> libavcodec/flacenc.c:    s->options.max_prediction_order = ((int[]){  3,  4,  4,  6,  8,  8,  8,  8, 12, 12, 12,
> libavcodec/flacenc.c:        s->options.prediction_order_method = ((int[]){ ORDER_METHOD_EST,    ORDER_METHOD_ES
> libavcodec/flacenc.c:        s->options.min_partition_order = ((int[]){  2,  2,  0,  0,  0,  0,  0,  0,  0,  0, 
> libavcodec/flacenc.c:        s->options.max_partition_order = ((int[]){  2,  2,  3,  3,  3,  8,  8,  8,  8,  8, 
> libavcodec/g726.c:    avctx->frame_size = ((int[]){ 4096, 2736, 2048, 1640 })[c->code_size - 2];
> libavcodec/tiffenc.c:    flip ^= ((int[]) { 0, 0, 0, 1, 3, 3 })[type];
> libavfilter/af_aresample.c:        out_samplerates = ff_make_format_list((int[]){ out_rate, -1 });
> libavfilter/af_aresample.c:        out_formats = ff_make_format_list((int[]){ out_format, -1 });
> libavformat/mov.c:    st->codec->channels = ((int[]){2,1,2,3,3,4,4,5})[acmod] + lfeon;

As said, it only has trouble with things that aren't compile-time
constants, just as if they were static initializers.
aresample is the only one that uses a runtime value.
For example this works just fine (except that it breaks the code of
course):
out_samplerates = ff_make_format_list((int[]){ 0, -1 });
Note that you actually missed the biggest chunk, and that is our
pix_fmt/sample format lists, e.g.:
    .pix_fmts       = (const enum AVPixelFormat[]) {
    AV_PIX_FMT_VDPAU_H264,
    AV_PIX_FMT_NONE},

They all aren't any issue it seems.