[XL] Change in coreboot[main]: lib/jpeg: Replace decoder with Wuffs' implementation

[Nigel Tao (Code Review)]

Attention is currently required from: Martin L Roth, Patrick Georgi, Tim Wawrzynczak.

Nigel Tao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78271?usp=email )

Change subject: lib/jpeg: Replace decoder with Wuffs' implementation
......................................................................

Patch Set 2:

(6 comments)

Patchset:

PS2: 
I dropped some drive-by comments. Feel free to CC me on future Wuffs-related coreboot reviews.

File src/lib/jpeg.c:

https://review.coreboot.org/c/coreboot/+/78271/comment/c7ee49d3_3a824457 :
PS2, Line 9: #include <stdint.h>
FYI, I don't know how strict you coreboot folks are about #include style, but this is possibly unnecessary, because `#include "../vendorcode/wuffs/wuffs-v0.4.c"` will pull in `stdint.h`.

https://review.coreboot.org/c/coreboot/+/78271/comment/2ce117d3_87a08ee2 :
PS2, Line 97: 	uint64_t workbuf_len_min_incl = wuffs_jpeg__decoder__workbuf_len(&dec).min_incl;
etc.min_incl (at the end of the statement) is fine and will work. But FYI, using max_incl instead turns the knob from "optimize for less memory usage" to "optimize for more decode speed".

That's in theory. In practice, as of version 0.4 today, the minimum and maximum settings are the same number. But versions 0.4 in the future might implement multiple algorithms and let the caller configure.

https://review.coreboot.org/c/coreboot/+/78271/comment/58b94e18_929ca369 :
PS2, Line 99: 	if (workbuf_array == 0) {
```
if ((workbuf_array == 0) && workbuf_len_min_incl) {
```

might be more robust, in case a future Wuffs version drops the minimum work buffer memory usage down to zero.

https://review.coreboot.org/c/coreboot/+/78271/comment/6b9d61a4_1430854e :
PS2, Line 107: 	if (status.repr) {
I think you want a `free(workbuf_array)` before this line, as the Wuffs JPEG decoder no longer needs the work buffer, but I don't know coreboot's malloc/free at all (e.g. what's a ramstage).

https://review.coreboot.org/c/coreboot/+/78271/comment/9d4901ff_dfb11605 :
PS2, Line 108: 		return JPEG_DECODE_FAILED;
FYI, if you want separate error codes to distinguish e.g. "invalid argument" from "invalid JPEG" from "unsupported JPEG" (e.g. strictly speaking, the JPEG spec allows for 12-bit precision, instead of 8-bit, but these are very, very rare and many JPEG libraries, including Wuffs, do not support them), then:

A `wuffs_base__status` is just a `const char*`, wrapped in a struct (so that, in C++, it can have syntactic-sugar methods):

```
typedef struct wuffs_base__status__struct {
  const char* repr;

#ifdef __cplusplus
  // etc
#endif  // __cplusplus

} wuffs_base__status;
```

The `const char*` is always a C string literal and can be compared by `==`, not just by `strcmp`. Wuffs' general 'base' errors and JPEG-specific errors have declarations like this:

```
// This is an excerpt. For a full list:
// grep ^extern.const.char.wuffs_.*__error_ wuffs-v0.4.c

extern const char wuffs_base__error__bad_argument[];
extern const char wuffs_base__error__bad_receiver[];
extern const char wuffs_base__error__bad_wuffs_version[];
extern const char wuffs_base__error__unsupported_image_dimension[];

// etc

extern const char wuffs_jpeg__error__bad_dht_marker[];
extern const char wuffs_jpeg__error__bad_dqt_marker[];
extern const char wuffs_jpeg__error__bad_dri_marker[];
extern const char wuffs_jpeg__error__bad_sof_marker[];
extern const char wuffs_jpeg__error__bad_sos_marker[];
extern const char wuffs_jpeg__error__missing_huffman_table[];
extern const char wuffs_jpeg__error__missing_quantization_table[];
extern const char wuffs_jpeg__error__truncated_input[];
extern const char wuffs_jpeg__error__unsupported_precision_12_bits[];
extern const char wuffs_jpeg__error__unsupported_precision_16_bits[];
extern const char wuffs_jpeg__error__unsupported_precision[];
extern const char wuffs_jpeg__error__unsupported_scan_count[];
```

A NULL `const char*` means OK. So, here, you could do something like

```
if (!status.repr) {
  // No-op.
} else if (status.repr == wuffs_jpeg__error__truncated_input) {
  return SOME_SPECIFIC_ERROR_CODE;
} else if ((status.repr == wuffs_jpeg__error__unsupported_precision_12_bits) ||
    (status.repr == wuffs_jpeg__error__unsupported_precision_16_bits)) {
  return SOME_OTHER_SPECIFIC_ERROR_CODE;
} else {
  return SOME_GENERAL_ERROR_CODE;
}
```

Again, that's all if you care to distinguish different errors. If you don't care, `return JPEG_DECODE_FAILED` is fine.

---

Separately, if your `printk` can handle `%s` instead of `%d`, your `jpeg_decode` function could return `const char*` instead of `int`. The actual error string definitions look like this:

```
const char wuffs_jpeg__error__unsupported_precision_12_bits[] = "#jpeg: unsupported precision (12 bits)";
```

and the code here changes from

```
if (status.repr) {
  return JPEG_DECODE_FAILED;
}
return 0;
```

to

```
return status.repr;
```