[FFmpeg-devel] [PATCH v1 1/4] avcodec/webp: move definitions into header

[Thilo Borgmann]

Am 14.06.23 um 12:08 schrieb Andreas Rheinhardt:
> Thilo Borgmann:
>> ---
>>   libavcodec/webp.c | 17 +--------------
>>   libavcodec/webp.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 56 insertions(+), 16 deletions(-)
>>   create mode 100644 libavcodec/webp.h
>>
>> diff --git a/libavcodec/webp.c b/libavcodec/webp.c
>> index d35cb66f8d..15152ec8fb 100644
>> --- a/libavcodec/webp.c
>> +++ b/libavcodec/webp.c
>> @@ -52,22 +52,7 @@
>>   #include "thread.h"
>>   #include "tiff_common.h"
>>   #include "vp8.h"
>> -
>> -#define VP8X_FLAG_ANIMATION             0x02
>> -#define VP8X_FLAG_XMP_METADATA          0x04
>> -#define VP8X_FLAG_EXIF_METADATA         0x08
>> -#define VP8X_FLAG_ALPHA                 0x10
>> -#define VP8X_FLAG_ICC                   0x20
>> -
>> -#define MAX_PALETTE_SIZE                256
>> -#define MAX_CACHE_BITS                  11
>> -#define NUM_CODE_LENGTH_CODES           19
>> -#define HUFFMAN_CODES_PER_META_CODE     5
>> -#define NUM_LITERAL_CODES               256
>> -#define NUM_LENGTH_CODES                24
>> -#define NUM_DISTANCE_CODES              40
>> -#define NUM_SHORT_DISTANCES             120
>> -#define MAX_HUFFMAN_CODE_LENGTH         15
>> +#include "webp.h"
>>   
>>   static const uint16_t alphabet_sizes[HUFFMAN_CODES_PER_META_CODE] = {
>>       NUM_LITERAL_CODES + NUM_LENGTH_CODES,
>> diff --git a/libavcodec/webp.h b/libavcodec/webp.h
>> new file mode 100644
>> index 0000000000..90baa71182
>> --- /dev/null
>> +++ b/libavcodec/webp.h
>> @@ -0,0 +1,55 @@
>> +/*
>> + * WebP image format definitions
>> + * Copyright (c) 2020 Pexeso Inc.
>> + *
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +/**
>> + * @file
>> + * WebP image format definitions.
>> + */
>> +
>> +#ifndef AVCODEC_WEBP_H
>> +#define AVCODEC_WEBP_H
>> +
>> +#define VP8X_FLAG_ANIMATION             0x02
>> +#define VP8X_FLAG_XMP_METADATA          0x04
>> +#define VP8X_FLAG_EXIF_METADATA         0x08
>> +#define VP8X_FLAG_ALPHA                 0x10
>> +#define VP8X_FLAG_ICC                   0x20
>> +
>> +#define ANMF_DISPOSAL_METHOD            0x01
>> +#define ANMF_DISPOSAL_METHOD_UNCHANGED  0x00
>> +#define ANMF_DISPOSAL_METHOD_BACKGROUND 0x01
>> +
>> +#define ANMF_BLENDING_METHOD            0x02
>> +#define ANMF_BLENDING_METHOD_ALPHA      0x00
>> +#define ANMF_BLENDING_METHOD_OVERWRITE  0x02
>> +
>> +#define MAX_PALETTE_SIZE                256
>> +#define MAX_CACHE_BITS                  11
>> +#define NUM_CODE_LENGTH_CODES           19
>> +#define HUFFMAN_CODES_PER_META_CODE     5
>> +#define NUM_LITERAL_CODES               256
>> +#define NUM_LENGTH_CODES                24
>> +#define NUM_DISTANCE_CODES              40
>> +#define NUM_SHORT_DISTANCES             120
>> +#define MAX_HUFFMAN_CODE_LENGTH         15
>> +
>> +
>> +#endif /* AVCODEC_WEBP_H */
> 
> 1. Some of these defines (like MAX_CACHE_BITS) are unused now and seem
> to stay that way in your patchset.
> 2. If you move defines in a header, you need to ensure that they are
> properly prefixed so that no conflicts can arise. This is particularly
> true of defines like VP8X_FLAG_* whose name actually indicates that they
> belong into a vp8.h.
> 3. It seems that your patchset only includes this header in webp.c; they
> are not used outside of it. So there is no need for a header.

All true, I believe. Having it in a header originates from the original patchset 
including a parser where the header is then used as well. Because the parser did 
not get any feedback I posted only the decoder here and kept the header file 
anyway to keep the decoder patch smaller. I can keep it in there if you wish, 
though - then the need for prefixing becomes void, IIUC.

I think it makes no sense to drap these defines into vp8.h as I believe they 
make sense only in the VP8-in-WebP case.

v2 will include it in the decoder then, thx!

-Thilo