[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