[FFmpeg-devel] libavcodec : add psd image file decoder

[Andreas Cadhalpun]

On 18.11.2016 18:27, Martin Vignali wrote:
> From c41e6c79ed42478a1658c8922d19c832d3a89424 Mon Sep 17 00:00:00 2001
> From: Martin Vignali <martin.vignali at gmail.com>
> Date: Fri, 18 Nov 2016 18:18:20 +0100
> Subject: [PATCH 1/2] libavcodec : add decoder for Photoshop PSD image file.
                                                                            ^
image file*s* and no dot at the end

> Decode the Image Data Section (who contain merge picture).
                                 ^
(which contains merged pictures)

> Support RGB/A and Grayscale/A in 8bits and 16 bits by channel.
                                                     ^
per channel

> Support uncompress and rle compression in Image Data Section.
          ^
decompression

> ---
>  Changelog              |   1 +
>  doc/general.texi       |   2 +
>  libavcodec/Makefile    |   1 +
>  libavcodec/allcodecs.c |   1 +
>  libavcodec/avcodec.h   |   1 +
>  libavcodec/psd.c       | 428 +++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 434 insertions(+)
>  create mode 100644 libavcodec/psd.c

Have you read doc/developers.texi, section "New codecs or formats checklist"?
 * needs minor version bump
 * codec should be added to libavcodec/codec_desc.c

Also a fate test would be nice.

> diff --git a/libavcodec/psd.c b/libavcodec/psd.c
> new file mode 100644
> index 0000000..90ee337
> --- /dev/null
> +++ b/libavcodec/psd.c
> @@ -0,0 +1,428 @@
[...]
> +    s->height = bytestream2_get_be32(&s->gb);
> +
> +    if ((s->height < 1) || (s->height > 30000)) {

Why 30000?
ff_set_dimensions already checks for sane dimensions.

> +        av_log(s->avctx, AV_LOG_ERROR, "Invalid height %d.\n", s->height);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    s->width = bytestream2_get_be32(&s->gb);
> +    if ((s->width < 1) || (s->width > 30000)) {
> +        av_log(s->avctx, AV_LOG_ERROR, "Invalid width %d.\n", s->width);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    if ((ret = ff_set_dimensions(s->avctx, s->width, s->height)) < 0)
> +        return ret;
> +
> +    s->channel_depth = bytestream2_get_be16(&s->gb);
> +
> +    color_mode = bytestream2_get_be16(&s->gb);
> +    switch (color_mode) {
> +    case 0:
> +        s->color_mode = PSD_BITMAP;
> +        break;
> +    case 1:
> +        s->color_mode = PSD_GRAYSCALE;
> +        break;
> +    case 2:
> +        s->color_mode = PSD_INDEXED;
> +        break;
> +    case 3:
> +        s->color_mode = PSD_RGB;
> +        break;
> +    case 4:
> +        s->color_mode = PSD_CMYK;
> +        break;
> +    case 7:
> +        s->color_mode = PSD_MULTICHANNEL;
> +        break;
> +    case 8:
> +        s->color_mode = PSD_DUOTONE;
> +        break;
> +    case 9:
> +        s->color_mode = PSD_LAB;
> +        break;
> +    default:
> +        av_log(s->avctx, AV_LOG_ERROR, "Unknown color mode %d.\n", color_mode);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    /* color map data */
> +    len_section = bytestream2_get_be32(&s->gb);
> +    if (len_section < 0)

Please add an error message.

> +        return AVERROR_INVALIDDATA;
> +
> +    if (bytestream2_get_bytes_left(&s->gb) < (len_section + 4)) { /* section and len next section */
> +        av_log(s->avctx, AV_LOG_ERROR, "Incomplete file.\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +    bytestream2_skip(&s->gb, len_section);
> +
> +    /* image ressources */
> +    len_section = bytestream2_get_be32(&s->gb);
> +    if (len_section < 0)

Here too.

> +        return AVERROR_INVALIDDATA;
> +
> +    if (bytestream2_get_bytes_left(&s->gb) < (len_section + 4)) { /* section and len next section */
> +        av_log(s->avctx, AV_LOG_ERROR, "Incomplete file.\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +    bytestream2_skip(&s->gb, len_section);
> +
> +    /* layers and masks */
> +    len_section = bytestream2_get_be32(&s->gb);
> +    if (len_section < 0)

And here.

[...]
> +static int decode_frame(AVCodecContext *avctx, void *data,
> +                        int *got_frame, AVPacket *avpkt)
> +{
> +    int ret;
> +    uint8_t *ptr;
> +    const uint8_t * ptr_data;
                      ^
nit: no space between * and variable name. (Also elsewhere.)

> +    int index_out, c, y, x, p;
> +    uint8_t eq_channel[4] = {2,0,1,3};;/* RGBA -> GBRA channel order */
                                         ^
One ';' too much, use a space instead.

> +    uint8_t plane_number;
> +
> +    AVFrame *picture = data;
> +
> +    PSDContext *s = avctx->priv_data;
> +    s->avctx     = avctx;
> +    s->channel_count = 0;
> +    s->channel_depth = 0;
> +    s->tmp           = NULL;
> +    s->line_size     = 0;
> +
> +    bytestream2_init(&s->gb, avpkt->data, avpkt->size);
> +
> +    if ((ret = decode_header(s)) < 0)
> +        return ret;
> +
> +    s->pixel_size = s->channel_depth >> 3;/* in byte */
> +    s->line_size = s->width * s->pixel_size;
> +    s->uncompressed_size = s->line_size * s->height * s->channel_count;
> +
> +    switch (s->color_mode) {
> +    case PSD_RGB:
> +        if (s->channel_count == 3) {
> +            if (s->channel_depth == 8) {
> +                avctx->pix_fmt = AV_PIX_FMT_GBRP;
> +            } else if (s->channel_depth == 16) {
> +                avctx->pix_fmt = AV_PIX_FMT_GBRP16BE;
> +            } else {
> +                avpriv_report_missing_feature(avctx, "channel depth %d unsupported for rgb", s->channel_depth);

This should just say "Channel depth %d for rgb", because this function will
continue the text with " is not implemented.". Same for the similar cases below.

> From 1b85107d53ab4e5748156e8e84cb8fbe26317700 Mon Sep 17 00:00:00 2001
> From: Martin Vignali <martin.vignali at gmail.com>
> Date: Fri, 18 Nov 2016 18:18:36 +0100
> Subject: [PATCH 2/2] libavformat : add Photoshop PSD file.
                                                       ^
demuxer

And this also needs a minor version bump.

On 18.11.2016 23:41, Martin Vignali wrote:
> You can find sample here :
> https://we.tl/BHKIQCDZZm

Thanks. I fuzz-tested the demuxer/decoder and didn't find any problems.
However, all the rle samples seem to not decode entirely correct, instead
containing blue/white rectangular areas.

Best regards,
Andreas