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

[Dai, Jianhui J]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Ronald S.
> Bultje
> Sent: Wednesday, November 8, 2023 4:26 AM
> 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 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?

The smaller delta patch to export the variable:  https://patchwork.ffmpeg.org/project/ffmpeg/patch/DM6PR11MB268186349E600824E1577DFDB1A8A@DM6PR11MB2681.namprd11.prod.outlook.com/
Personally, I prefer to limit the static data only in vp8.c. 

> 
> Ronald
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request at ffmpeg.org with
> subject "unsubscribe".