[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#51296: [PATCH] Add WebP format support
From: |
Stefan Kangas |
Subject: |
bug#51296: [PATCH] Add WebP format support |
Date: |
Thu, 21 Oct 2021 11:36:45 -0700 |
Eli Zaretskii <eliz@gnu.org> writes:
> Thanks, now only a few minor issues remain:
All your comments should be fixed, see below for details. I've also
attached an updated patch. Thank you for reviewing.
> That's OK, but I believe you should also add WEBP to the list of Emacs
> configuration features around line 5885 in configure.ac, so that WebP
> support could be reported by system-configuration-features.
Ah, right. Fixed.
> Our style is to separate the '*' from the type, like this:
>
> DEF_DLL_FN (int, WebPGetInfo, (const uint8_t *, size_t, int *, int *));
> ^^ ^^ ^^
> Also note that there's no need to use names of parameters in
> prototypes, they just make the code longer, but don't add anything
> useful. So I removed them in the above.
Fixed.
>> + if (!(library = w32_delayed_load (Qwebp)))
>> + return 0;
> ^^^^^^^^
> "return false" is more consistent.
Fixed.
>> + contents = (uint8_t*) SSDATA (specified_data);
>
> Space before '*' again. Also, is the type cast really needed? If
> not, it is better to drop it.
Fixed the style issue.
The cast fixes this warning, so I kept it and added a comment saying
"Casting avoids a GCC warning":
image.c: In function ‘webp_load’:
image.c:8878:16: warning: pointer targets in assignment from ‘char *’
to ‘uint8_t *’ {aka ‘unsigned char *’} differ in signedness
[-Wpointer-sign]
8878 | contents = SSDATA (specified_data);
| ^
>> + /* Validate the WebP image header. */
>> + if (!WebPGetInfo (contents, size, NULL, NULL))
>> + {
>> + if (!NILP (specified_data))
>> + image_error ("Not a WebP file: `%s'", file);
>> + else
>> + image_error ("Invalid WebP data");
>
> This last error message could IMO be more useful, if it said something
> like "Non-WebP header in WebP image data".
I went with: "Invalid header in WebP image data".
>> + image_error ("Error when interpreting WebP data: `%s'", file);
>
> Suggest to say "Error when interpreting WebP data from file `%s'"
> instead, otherwise it may not be clear to the user what is that string
> after the colon.
>
>> + image_error ("Error when interpreting WebP data");
>
> I'd suggest "Error when interpreting WebP image data".
Yes, that's better. Fixed.
0001-Add-WebP-format-support.patch
Description: Text Data
- bug#51296: [PATCH] Add WebP format support, (continued)
- bug#51296: [PATCH] Add WebP format support, Stefan Kangas, 2021/10/20
- bug#51296: [PATCH] Add WebP format support, Eli Zaretskii, 2021/10/20
- bug#51296: [PATCH] Add WebP format support, Stefan Kangas, 2021/10/20
- bug#51296: [PATCH] Add WebP format support, Eli Zaretskii, 2021/10/20
- bug#51296: [PATCH] Add WebP format support, Eli Zaretskii, 2021/10/20
- bug#51296: [PATCH] Add WebP format support, Stefan Kangas, 2021/10/20
- bug#51296: [PATCH] Add WebP format support, Eli Zaretskii, 2021/10/20
- bug#51296: [PATCH] Add WebP format support, Stefan Kangas, 2021/10/20
- bug#51296: [PATCH] Add WebP format support, Stefan Kangas, 2021/10/20
- bug#51296: [PATCH] Add WebP format support, Eli Zaretskii, 2021/10/21
- bug#51296: [PATCH] Add WebP format support,
Stefan Kangas <=
- bug#51296: [PATCH] Add WebP format support, Eli Zaretskii, 2021/10/21
- bug#51296: [PATCH] Add WebP format support, Stefan Kangas, 2021/10/21
- bug#51296: [PATCH] Add WebP format support, Eli Zaretskii, 2021/10/22
- bug#51296: [PATCH] Add WebP format support, Stefan Kangas, 2021/10/22
- bug#51296: [PATCH] Add WebP format support, Eli Zaretskii, 2021/10/22
- bug#51296: [PATCH] Add WebP format support, Eli Zaretskii, 2021/10/22
- bug#51296: [PATCH] Add WebP format support, Stefan Kangas, 2021/10/22