[FFmpeg-devel] [PATCH 2/2] Adding closed caption decoder

[wm4]

On Sun, 4 Jan 2015 22:49:16 +0100
Hendrik Leppkes <h.leppkes at gmail.com> wrote:

> On Sun, Jan 4, 2015 at 6:41 PM, wm4 <nfxjfg at googlemail.com> wrote:
> 
> > On Sun, 4 Jan 2015 17:47:20 +0100
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> >
> > > > >> +};
> > > > >> +
> > > > >> +
> > > > >> +typedef struct CCaptionSubContext {
> > > > >> +    AVClass *class;
> > > > >> +    int parity_table[256];
> > > > > this can be a static uint8_t table
> > > > >
> > > > I don't think static variable in structure are allowed in c language
> > > > that is cpp thing.
> > > >
> > > > If you meant to remove that table from structure, then too its
> > > > not efficient, we have to make parity table every time decode
> > > > function is called.
> > >
> > > the table is constant and does not change, theres no need to have
> > > a copy of it in each context or to "make it every time decode is
> > > called"
> > > a simple static uint8_t parity_table[256];
> > > or even
> > > static const uint8_t parity_table[256] = {...}
> > >
> >
> > IMO such global writable state is bad style and should be avoided if
> > possible. Even if it's only 256 bytes in this case. What's wrong with
> > keeping it in the context?
> >
> >
> I thought the point was that it was constant?

It is... maybe it would be best to put the table contents as constant
in the code, instead of computing it at runtime, as it was suggested by
Niedermayer. I just think it's bad to keep global mutable state around,
especially for such tiny tables (and complaining about it in new
patches is much easier than going and removing all the existing cases
in lavc).