Re: [FFmpeg-devel] [PATCH 1/2] avcodec/jpegtables: remove duplicate luma and chroma quantization tables

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Peter Ross:
> On Fri, Oct 14, 2022 at 05:19:44PM +0200, Andreas Rheinhardt wrote:
>> Peter Ross:
>>> Duplicates of the standard JPEG quantization tables were found in the
>>> AGM, MSS34(dsp), NUV and VP31 codecs. This patch elimates those duplicates,
>>> placing a single copy in jpegtables.
>>> ---
> 
>> 1. mss34dsp now uses jpegtables, yet it does not have a dependency on
>> it. Instead you seem to rely on all the users of mss34dsp to have a
>> dependency on jpegtables, yet this is not true for all of them. You will
>> get linking failures with --disable-everything --enable-decoder=msa1.
> 
> there is a OBJS-$(CONFIG_JPEGTABLES) += line in avcodec/Makefile that
> takes care of this. i tested this patch under few different configurations
> and observed no build errors.
> 

I am sorry, I missed the '+mss34dsp_select="jpegtables"' line from your
patch. So my first sentence above was simply bullshit.

>> 2. The fact that you need to add jpegtables to configure for almost all
>> components that use the jpeg quant tables means that it is not really
>> appropriate to put the jpeg quant tables into the same files as the jpeg
>> huff tables.
>> 3. The jpeg huff tables are duplicated into libavformat for shared
>> builds (because the overhead of exporting them exceeds the size gains
>> from not duplicating them); yet when one uses --enable-shared and
>> --enable-static at the same time, it might be that libavformat.a is
>> linked to libavcodec.so and therefore has no access to libavcodec's
>> internal symbols like the jpegtables, so we have to duplicate the
>> jpegtables into libavformat.a in this case. But if one links using
>> libavformat.a and libavcodec.a with both containing the jpeg huffman
>> tables, then one will get a linker error with this patch: The jpeg
>> huffman tables in libavformat will be pulled in by the libavformat
>> component needing them. With this patch libavcodec/jpegtables.o will
>> also be pulled in. But it also contains the jpeg tables already provided
>> by lavf/jpegtables.o, so you will get a multiple definition error.
>> In contrast to this, before this patch, lavc/jpegtables.o would not be
>> pulled in, because all dependencies to the mjpeg huffman tables will be
>> satisfied by lavf/jpegtabes.o.
>> So to summarize: If one duplicates stuff in multiple libraries, the
>> object files must really provide the exact same symbols; not more, not less.
> 
> wasn't aware jpegtabs.h was duplicated into libavformat.
> 

I said so on IRC yesterday. It's the reason the jpegtabs header exists.

>> The last two points imply that these tables should be moved to a file of
>> their own. Btw: I don't think that a configure subsystem is appropriate
>> for this (a single file with some tables is not really a subsystem);
>> actually I don't think that the current jpegtables subsystem is
>> appropriate at all.
> 
> will do
> 

If you do, please mark the declarations as internal so that the compiler
still knows that these tables are in the same DSO.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".