[FFmpeg-devel] [PATCH v9 1/5] avcodec/jpegxl: add Jpeg XL image codec and parser

[Leo Izen]

On 3/23/22 10:45, Andreas Rheinhardt wrote:
> Leo Izen:
> + /* any other box is skipped at this point */
>> +        AV_WB32(tag_str, tag);
>> +        av_log(avctx, AV_LOG_VERBOSE, "skipping jxl container box: %s\n", tag_str);
> 1. tag_str is potentially not-zero terminated.
> 2. If tag_str contains a \0, it might get truncated; it would be better
> to just report it as hex with %X or so.
> 3. And actually I don't think that this should be reported at all.
If I change the report level to AV_LOG_DEBUG and report it as hex, does 
this work?
>> +static uint64_t jpegxl_get_bits(void *avctx, JpegXLParseContext *jxlr, int bits)
>> +{
>> +    if (jxlr->box_size) {
>> +        if (bits > jxlr->box_size) {
>> +            int remaining = jxlr->box_size;
>> +            uint64_t ret = jpegxl_get_bits(avctx, jxlr, remaining);
>> +            /* go to the next box */
>> +            int status = jpegxl_skip_boxes(avctx, jxlr);
>> +            if (status)
>> +                return 0;
>> +            ret |= jpegxl_get_bits(avctx, jxlr, bits - remaining) << remaining;
> What guarantees that there is not a sequence of boxes with a payload of
> 1 byte, so that a single read can span more than two boxes?
>
> And does the file format really allow to split the payload into
> different boxes at arbitrary positions?
>
Nothing guarantees it. If it does, the second call to jpegxl_get_bits 
will recurse. Since you can only request 64 bits at once and all jxlp 
boxes are at least one byte of payload, this has worst-case-scenario of 
8 calls for a 64 bits request. And unfortunately, it does allow the 
payload to be split at arbitrary positions.

>> +    *width = w, *height = h;
>> +    return 0;

 >Why does this pretend to be able to fail when it just can't?

I was going to move the size validity check to these, but I forgot. I 
will do that next revision of the patch.

>> +    *poutbuf = buf + i;
>> +    *poutbuf_size = buf_size - i;

 > Seems like the parser is discarding some data here (if i != 0).

That's the idea. It discards data that precedes the start of the frame. 
Is it not supposed to do this?


Leo Izen (thebombzen)