[FFmpeg-devel] [PATCH v2 3/3] libavcodec/jpeg2000dec: Support for PPM marker

[Michael Niedermayer]

On Sat, Jul 18, 2020 at 06:46:22PM +0530, gautamramk at gmail.com wrote:
> From: Gautam Ramakrishnan <gautamramk at gmail.com>
> 
> This patch adds support for PPM marker for JPEG2000
> decoder. It allows the samples p1_03.j2k and p1_05.j2k
> to be decoded.
> ---
>  libavcodec/jpeg2000dec.c | 111 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 101 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
> index 5e9e97eb6a..e37f834afe 100644
> --- a/libavcodec/jpeg2000dec.c
> +++ b/libavcodec/jpeg2000dec.c
> @@ -71,6 +71,7 @@ typedef struct Jpeg2000POC {
>  typedef struct Jpeg2000TilePart {
>      uint8_t tile_index;                 // Tile index who refers the tile-part
>      const uint8_t *tp_end;
> +    GetByteContext header_tpg;          // bit stream of header if PPM header is used
>      GetByteContext tpg;                 // bit stream in tile-part
>  } Jpeg2000TilePart;
>  
> @@ -102,6 +103,13 @@ typedef struct Jpeg2000DecoderContext {
>      uint8_t         cbps[4];    // bits per sample in particular components
>      uint8_t         sgnd[4];    // if a component is signed
>      uint8_t         properties[4];
> +
> +    uint8_t         has_ppm;
> +    uint8_t         *packed_headers; // contains packed headers. Used only along with PPM marker
> +    int             packed_headers_size;
> +    GetByteContext  packed_headers_stream;

> +    uint8_t         in_tile_headers;

in_tile_headers could be moved into a seperate patch before the ppm addition
it could also be used for other things than ppm



> +
>      int             cdx[4], cdy[4];
>      int             precision;
>      int             ncomponents;
> @@ -928,6 +936,31 @@ static int get_plt(Jpeg2000DecoderContext *s, int n)
>      return 0;
>  }
>  
> +static int get_ppm(Jpeg2000DecoderContext *s, int n)
> +{
> +    void *new;
> +
> +    if (n < 3) {
> +        av_log(s->avctx, AV_LOG_ERROR, "Invalid length for PPM data.\n");
> +        return AVERROR_INVALIDDATA;
> +    }

> +    s->has_ppm = 1;

Is there a reason has_ppm is set before all error checks ?
this leaves the possibility for has_ppm to be set but get_ppm failing
it seems more consistant to me to only set it if get_ppm succeeded

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If the United States is serious about tackling the national security threats 
related to an insecure 5G network, it needs to rethink the extent to which it
values corporate profits and government espionage over security.-Bruce Schneier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200719/ffd16e2b/attachment.sig>