[FFmpeg-devel] [PATCH v5] avcodec/cbs_vp8: Add support for VP8 codec bitstream

[Ronald S. Bultje]

Hi,

On Tue, Nov 7, 2023 at 12:01 AM Dai, Jianhui J <
jianhui.j.dai-at-intel.com at ffmpeg.org> wrote:

>
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > Ronald S. Bultje
> > Sent: Monday, November 6, 2023 8:08 PM
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel at ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH v5] avcodec/cbs_vp8: Add support for
> > VP8 codec bitstream
> >
> > Hi,
> >
> > On Mon, Nov 6, 2023 at 7:07 AM Ronald S. Bultje <rsbultje at gmail.com>
> > wrote:
> >
> > > On Sun, Nov 5, 2023 at 11:13 PM Dai, Jianhui J <
> > > jianhui.j.dai-at-intel.com at ffmpeg.org> wrote:
> > >
> > >> > > +static const uint8_t vp8_token_update_probs[4][8][3][11] = {
> > >> >
> > >> > It would be nice if these symbols could be re-used from the
> > >> > existing vp8 native decoder, instead of duplicating them? Both
> > >> > source + binary size
> > >> are
> > >> > relevant here.
> > >>
> > >> Including vp8data.h would introduce many unwanted static tables other
> > >> than vp8_token_update_probs and increase the binary size.
> > >> As suggested in patch v3, it is better to use local defined
> > >> vp8_token_update_probs.
> > >>
> > >
> > > I didn't mean to include vp8data.h. I mean to include a new *.h that
> > > declares extern const uint8_t vp8_token_update_probs[][][][] and move
> > > said table into a new *.c file. The point is to prevent symbol
> duplication.
> > >
> >
> > And to elaborate further, in case it's unclear: the symbol move means the
> > native VP8 decoder would include this probability table using the same
> new
> > mechanism also. It would no longer exist in vp8data.h.
>
> Right.
> I made another change to reorganize the vp8data.[c|h] and only export `
> ff_vp8_dct_cat_prob` and `ff_vp8_token_default_probs`.
> Please take a look.
>

I'm not sure it's a good idea to move all tables back into vp8.c. There's a
reason we added it in a separate header file, so that "large random tables
with numbers" don't obfuscate the actual source code file. Or to say it
diffrently: you could probably have accomplished the same effect with a
much smaller diff... That's just my opinion though. Anyone else care about
any of this?

Ronald