[FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: Add support for placeholder passes, CAP, and CPF markers

[Tomas Härdin]

It seems this patch combines a lot of things that might be better to
split into separate patches for easier review

> @@ -382,6 +391,9 @@ static int get_siz(Jpeg2000DecoderContext *s)
>          } else if (ncomponents == 1 && s->precision == 8) {
>              s->avctx->pix_fmt = AV_PIX_FMT_GRAY8;
>              i = 0;
> +        } else if (ncomponents == 1 && s->precision == 12) {
> +            s->avctx->pix_fmt = AV_PIX_FMT_GRAY16LE;
> +            i = 0;

Could we handle 9 <= precision <= 16 while we're at it?

> @@ -408,6 +420,73 @@ static int get_siz(Jpeg2000DecoderContext *s)
>      s->avctx->bits_per_raw_sample = s->precision;
>      return 0;
>  }
> +/* get extended capabilities (CAP) marker segment */
> +static int get_cap(Jpeg2000DecoderContext *s, Jpeg2000CodingStyle
> *c)
> +{
> +    uint32_t Pcap;
> +    uint16_t Ccap_i[32] = { 0 };
> +    uint16_t Ccap_15;
> +    uint8_t P;
> +
> +    if (bytestream2_get_bytes_left(&s->g) < 6) {
> +        av_log(s->avctx, AV_LOG_ERROR, "Insufficient space for
> CAP\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    Pcap = bytestream2_get_be32u(&s->g);
> +    s->isHT = (Pcap >> (31 - (15 - 1))) & 1;
> +    for (int i = 0; i < 32; i++) {
> +        if ((Pcap >> (31 - i)) & 1)
> +            Ccap_i[i] = bytestream2_get_be16u(&s->g);
> +    }
> +    Ccap_15 = Ccap_i[14];
> +    if (s->isHT == 1) {
> +    av_log(s->avctx, AV_LOG_INFO, "This is an HTJ2K codestream.\n");
> +    // Bits 14-15
> +    switch ((Ccap_15 >> 14) & 0x3) {

Missing indentation

> +        case 0x3:
> +            s->Ccap15_b14_15 = HTJ2K_MIXED;
> +            break;
> +        case 0x1:
> +            s->Ccap15_b14_15 = HTJ2K_HTDECLARED;
> +            break;
> +        case 0x0:
> +            s->Ccap15_b14_15 = HTJ2K_HTONLY;
> +            break;
> +        default:
> +                av_log(s->avctx, AV_LOG_ERROR, "Unknown CCap
> value.\n");
> +            return AVERROR(EINVAL);
> +            break;
> +    }
> +    // Bit 13
> +    if ((Ccap_15 >> 13) & 1) {
> +        av_log(s->avctx, AV_LOG_ERROR, "MULTIHT set is not
> supported.\n");
> +        return AVERROR_PATCHWELCOME;
> +    }
> +    // Bit 12
> +    s->Ccap15_b12 = (Ccap_15 >> 12) & 1;
> +    // Bit 11
> +    s->Ccap15_b11 = (Ccap_15 >> 11) & 1;
> +    // Bit 5
> +    s->Ccap15_b05 = (Ccap_15 >> 5) & 1;
> +    // Bit 0-4
> +    P = Ccap_15 & 0x1F;
> +    if (!P)
> +        s->HT_MAGB = 8;
> +    else if (P < 20)
> +        s->HT_MAGB = P + 8;
> +    else if (P < 31)
> +        s->HT_MAGB = 4 * (P - 19) + 27;
> +    else
> +        s->HT_MAGB = 74;
> +
> +    if (s->HT_MAGB > 31) {
> +            av_log(s->avctx, AV_LOG_ERROR, "Available internal
> precision is exceeded (MAGB> 31).\n");
> +        return AVERROR_PATCHWELCOME;
> +    }
> +    }

Weird indentation

> @@ -802,6 +881,15 @@ static int read_crg(Jpeg2000DecoderContext *s,
> int n)
>      bytestream2_skip(&s->g, n - 2);
>      return 0;
>  }
> +
> +static int read_cpf(Jpeg2000DecoderContext *s, int n)
> +{
> +    if (bytestream2_get_bytes_left(&s->g) < (n - 2))
> +        return AVERROR_INVALIDDATA;
> +    bytestream2_skip(&s->g, n - 2);
> +    return 0;
> +}

Don't we already have code for skipping markers we don't care about?

> +
>  /* Tile-part lengths: see ISO 15444-1:2002, section A.7.1
>   * Used to know the number of tile parts and lengths.
>   * There may be multiple TLMs in the header.
> @@ -965,6 +1053,10 @@ static int init_tile(Jpeg2000DecoderContext *s,
> int tileno)
>              comp->roi_shift = s->roi_shift[compno];
>          if (!codsty->init)
>              return AVERROR_INVALIDDATA;
> +        if (s->isHT && (!s->Ccap15_b05) && (!codsty->transform))
> +            av_log(s->avctx, AV_LOG_WARNING, "Transformation = 0
> (lossy DWT) is found in HTREV HT set\n");
> +        if (s->isHT && s->Ccap15_b14_15 != (codsty->cblk_style >> 6)
> && s->Ccap15_b14_15 != HTJ2K_HTONLY)
> +            av_log(s->avctx, AV_LOG_WARNING, "SPcod/SPcoc value does
> not match bit 14-15 values of Ccap15\n");

Do you have samples demonstrating the need to accept such broken files?
If not then we should probably error out

> @@ -1704,7 +1989,7 @@ static int decode_cblk(const
> Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *cod
>                         Jpeg2000T1Context *t1, Jpeg2000Cblk *cblk,
>                         int width, int height, int bandpos, uint8_t
> roi_shift)
>  {
> -    int passno = cblk->npasses, pass_t = 2, bpno = cblk->nonzerobits
> - 1 + roi_shift;
> +    int passno = cblk->npasses, pass_t = 2, bpno = cblk->nonzerobits
> - 1;

Won't this break files with ROI? I see there's some ROI stuff further
down so maybe not

> @@ -2187,22 +2472,42 @@ static int
> jpeg2000_read_main_headers(Jpeg2000DecoderContext *s)
>              if (!s->tile)
>                  s->numXtiles = s->numYtiles = 0;
>              break;
> +        case JPEG2000_CAP:
> +            if (!s->ncomponents) {
> +                av_log(s->avctx, AV_LOG_WARNING, "CAP marker segment
> shall come after SIZ\n");

SHALL -> we should be able to safely reject. Similarly with the other
errors. Unless we know of an encoder that produces broken files then
there's no reason to be lenient. And if such a broken encoder exists we
could try to get it fixed

> @@ -112,6 +112,13 @@ typedef struct Jpeg2000DecoderContext {
>      Jpeg2000Tile    *tile;
>      Jpeg2000DSPContext dsp;
>  
> +    uint8_t         isHT; // HTJ2K?

Isn't it possible to mix Part 1 and HT in the same file? I know HTONLY
exists also

> @@ -406,6 +420,7 @@ static void recover_mag_sgn(StateVars *mag_sgn,
> uint8_t pos, uint16_t q, int32_t
>              E[n] = 32 - ff_clz(v[pos][i] | 1);
>              mu_n[n] = (v[pos][i] >> 1) + 1;
>              mu_n[n] <<= pLSB;
> +            mu_n[n] |= (1 << (pLSB - 1)); // Add 0.5 (reconstruction
> parameter = 1/2)

Aren't there conformance files to catch these kinds of errors? I
remember looked at J2K a while back, and I think we should add such
files to FATE

/Tomas