> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of Ronald S. > Bultje > Sent: Wednesday, November 8, 2023 4:26 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@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....@ffmpeg.org> wrote: > > > > > > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of > > > Ronald S. Bultje > > > Sent: Monday, November 6, 2023 8:08 PM > > > To: FFmpeg development discussions and patches <ffmpeg- > > > de...@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 <rsbul...@gmail.com> > > > wrote: > > > > > > > On Sun, Nov 5, 2023 at 11:13 PM Dai, Jianhui J < > > > > jianhui.j.dai-at-intel....@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/dm6pr11mb268186349e600824e1577dfdb1...@dm6pr11mb2681.namprd11.prod.outlook.com/ Personally, I prefer to limit the static data only in vp8.c. > > Ronald > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org > with > subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".