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

Martin Vignali martin.vignali at gmail.com
Sun Nov 20 18:57:29 EET 2016


> > 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
>

Correct locally


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

Not sure i understand, do you mean :
Support uncompress and rle decompression in Image Data Section.
?
Because currently, the psd reader, can manage uncompress or rle.


>
> > ---
> >  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
>
Correct locally.

>
> Also a fate test would be nice.
>
Il would add a fate test, with the samples (in the previous link, when the
path will be apply (to not mix, path and fate test).

>
> > 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.
>

Following adobe specs :
http://www.adobe.com/devnet-apps/photoshop/fileformatashtml/#50577409_pgfId-1055726
in a psd file, the width or height, can't be > to 30 000 pixels.


>
> > +        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.
>
Correct locally

>
> > +        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.
>
Correct locally

>
> > +        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.
>
Correct locally

>
> [...]
> > +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.)
>
Correct locally

>
> > +    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.
>
Correct locally

>
> > +    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.
>
Correct locally

>
> > 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.
>
Correct locally

>
> 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.
>
>
> Thanks for testing

Martin


More information about the ffmpeg-devel mailing list