diff mbox series

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

Message ID 20230608142029.16564-2-thilo.borgmann@mail.de
State New
Headers show
Series webp: add support for animated WebP decoding | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Thilo Borgmann June 8, 2023, 2:20 p.m. UTC 
---
 libavcodec/webp.c | 17 +--------------
 libavcodec/webp.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 16 deletions(-)
 create mode 100644 libavcodec/webp.h

Comments

James Zern June 14, 2023, 2:01 a.m. UTC | #1 
On Thu, Jun 8, 2023 at 7:20 AM Thilo Borgmann <thilo.borgmann@mail.de> wrote:
>
> ---
>  libavcodec/webp.c | 17 +--------------
>  libavcodec/webp.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 16 deletions(-)
>  create mode 100644 libavcodec/webp.h
>

lgtm.
Andreas Rheinhardt June 14, 2023, 10:08 a.m. UTC | #2 
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.

- Andreas
Thilo Borgmann June 14, 2023, 10:30 a.m. UTC | #3 
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
diff mbox series

Patch

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 */