| Message ID | CAJiLW2MhebbwUCu+Uwj+sv6zim5SZDMjSMON-akbDsikaEidXQ@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 18.11.2016 18:27, Martin Vignali wrote: > From c41e6c79ed42478a1658c8922d19c832d3a89424 Mon Sep 17 00:00:00 2001 > From: Martin Vignali <martin.vignali@gmail.com> > Date: Fri, 18 Nov 2016 18:18:20 +0100 > Subject: [PATCH 1/2] libavcodec : add decoder for Photoshop PSD image file. > > Decode the Image Data Section (who contain merge picture). > Support RGB/A and Grayscale/A in 8bits and 16 bits by channel. > Support uncompress and rle compression in Image Data Section. Can you share a few small samples? I'd like to fuzz test this decoder a bit. Best regards, Andreas
> > Can you share a few small samples? > I'd like to fuzz test this decoder a bit. > > You can find sample here : https://we.tl/BHKIQCDZZm Martin
On 18.11.2016 18:27, Martin Vignali wrote: > From c41e6c79ed42478a1658c8922d19c832d3a89424 Mon Sep 17 00:00:00 2001 > From: Martin Vignali <martin.vignali@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@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
2016-11-18 18:27 GMT+01:00 Martin Vignali <martin.vignali@gmail.com>: >> Did you see AV_PIX_FMT_GBRP, AV_PIX_FMT_GBRAP, AV_PIX_FMT_GBRP16 >> and AV_PIX_FMT_GBRAP16? >> > > New patch in attach use AV_PIX_FMT_GBRP, AV_PIX_FMT_GBRAP, > AV_PIX_FMT_GBRP16BE > and AV_PIX_FMT_GBRAP16BE for RGB mode > I doesn't find a planar, for YA, so there is now two way to store data > one for YA, and one for Gray and GBR. Sorry, I don't understand this sentence... >> And the more I think of it, the more likely it seems that (GBR) 9, 10, 12 >> and 14 >> could also be of use. >> > Don't think GBR 9, 10, 12, 14, can be use now for psd file. Then why do you return AVERROR_PATCHWELCOME for the non-8, non-16 case? > + 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); > + return AVERROR_PATCHWELCOME; > + } > + color_mode = bytestream2_get_be16(&s->gb); > + switch (color_mode) { Why is this not "s->color_mode = bytestream2_get_be16(&s->gb);" instead? > + compression = bytestream2_get_be16(&s->gb); > + switch (compression) { Same here: Why not "s->compression = bytestream2_get_be16(&s->gb);"? From the demuxer patch: > + if (p->buf_size < 26) > + return 0; Padding alone is 32, so this check is not necessary. Carl Eugen
> > 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@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
> > > I doesn't find a planar, for YA, so there is now two way to store data > > one for YA, and one for Gray and GBR. > > Sorry, I don't understand this sentence... > Not sure i understand what is your question. I use planar for RGB and Gray, bu for YA, i doesn't find a planar something like YAP and YAP_16BE. So i use the "interleaved" pixel format (YA and YA16BE). These two mode explain, the if/else part in the storage data part of decode_frame. > > >> And the more I think of it, the more likely it seems that (GBR) 9, 10, > 12 > >> and 14 > >> could also be of use. > >> > > Don't think GBR 9, 10, 12, 14, can be use now for psd file. > > Then why do you return AVERROR_PATCHWELCOME for the non-8, > non-16 case? > Not sure i understand. In this version, there is no support for 9, 10, 12, 14 bitdepth. (and i doesn't think psd file can store these kinds of pixel depth). But PSD support other pixel depth than 8, and 16 (like float for example). > > > + 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); > > + return AVERROR_PATCHWELCOME; > > + } > > > + color_mode = bytestream2_get_be16(&s->gb); > > + switch (color_mode) { > > Why is this not "s->color_mode = bytestream2_get_be16(&s->gb);" instead? > Seems more clean for me, considering color mode are not "continuous". so the switch case assign the value after checking it, using the enum. > > > + compression = bytestream2_get_be16(&s->gb); > > + switch (compression) { > > Same here: Why not "s->compression = bytestream2_get_be16(&s->gb);"? > > Correct locally > From the demuxer patch: > > + if (p->buf_size < 26) > > + return 0; > > Padding alone is 32, so this check is not necessary. > > > Correct locally Martin
I object to this patch for the reasons Rostislav outlined.. Kieran
Hello, 2016-11-20 18:05 GMT+01:00 Kieran Kunhya <kierank@obe.tv>: > I object to this patch for the reasons Rostislav outlined.. > Thanks for saying this now. I answer to Rotislav in July. No answer from him from this moment, and the answers from other people, doesn't seems to go in the same way. Martin
> However, all the rle samples seem to not decode entirely correct, instead > containing blue/white rectangular areas. > > > Sorry, forget to answer to that. The white/blue rect have been add by me to the sample file, in order to "enable" the rle compression of photoshop. So i think, the conversion is correct for RLE compression Martin
2016-11-20 18:04 GMT+01:00 Martin Vignali <martin.vignali@gmail.com>: >> >> > I doesn't find a planar, for YA, so there is now two way to store data >> > one for YA, and one for Gray and GBR. >> >> Sorry, I don't understand this sentence... > > Not sure i understand what is your question. > I use planar for RGB and Gray, bu for YA, i doesn't find a planar > something like YAP and YAP_16BE. > So i use the "interleaved" pixel format (YA and YA16BE). > These two mode explain, the if/else part in the storage data part of > decode_frame. Of course, thank you for explaining. (And reminding me that there are more indications that GRAY is RGB...) >> >> And the more I think of it, the more likely it seems that (GBR) 9, 10, >> >> 12 and 14 could also be of use. >> >> >> > Don't think GBR 9, 10, 12, 14, can be use now for psd file. >> >> Then why do you return AVERROR_PATCHWELCOME for the non-8, >> non-16 case? >> > Not sure i understand. > In this version, there is no support for 9, 10, 12, 14 bitdepth. (and i > doesn't think psd file can store these kinds of pixel depth). > But PSD support other pixel depth than 8, and 16 (like float for example). Ok. Thank you, Carl Eugen
2016-11-20 18:05 GMT+01:00 Kieran Kunhya <kierank@obe.tv>:
> I object to this patch for the reasons Rostislav outlined..
Since this version of the decoder does not only support
uncompressed images, I don't think the "reasons" are
still valid.
Iirc, FFmpeg does not support all features of practically
every standard implemented (think of yuv422 asp, h264
with transparency and the mentioned exr decoder), so
this doesn't seem like a strong argument either.
Carl Eugen
On 20 November 2016 at 17:05, Kieran Kunhya <kierank@obe.tv> wrote: > I object to this patch for the reasons Rostislav outlined.. > > Kieran > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > I actually don't mind the decoder anymore. The code looks simple and neat, and it would be nice to be able to view the few random psd files I have and which the decoder can open. No full decoding support is planned, which would require inhuman effort to complete even like 20% of anyway, so I doubt the decoder will grow into a huge mess. I mean viewing psd files is a big pain anyway, so eh, if the decoder can open a few of them and save people the effort of having gimp (which also doesn't support some psd features) without being a security disaster it's okay. When you think about it png is also a mess, its been extended to hell with nonstandard chunks, but we even have pretty good encoding support for the base of it. Not sure what the fuss is here, because there's no public standard?
On Mon, 21 Nov 2016 07:36:44 +0000 Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > On 20 November 2016 at 17:05, Kieran Kunhya <kierank@obe.tv> wrote: > > > I object to this patch for the reasons Rostislav outlined.. > > > > Kieran > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > I actually don't mind the decoder anymore. The code looks simple and neat, > and it would be nice to be able to view the few random psd files I have and > which the decoder can open. No full decoding support is planned, which > would require inhuman effort to complete even like 20% of anyway, so I > doubt the decoder will grow into a huge mess. I mean viewing psd files is a > big pain anyway, so eh, if the decoder can open a few of them and save > people the effort of having gimp (which also doesn't support some psd > features) without being a security disaster it's okay. When you think about > it png is also a mess, its been extended to hell with nonstandard chunks, > but we even have pretty good encoding support for the base of it. Not sure > what the fuss is here, because there's no public standard? Can we get support for .doc and .html too? I think these would be great features.
2016-11-21 12:04 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > On Mon, 21 Nov 2016 07:36:44 +0000 > Can we get support for .doc and .html too? I think these would be great > features. Maybe such comments are not against our policy (that you requested so much), in any case they are not useful. Carl Eugen
On Mon, 21 Nov 2016 12:48:31 +0100 Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2016-11-21 12:04 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > > On Mon, 21 Nov 2016 07:36:44 +0000 > > > Can we get support for .doc and .html too? I think these would be great > > features. > > Maybe such comments are not against our policy (that you requested so > much), in any case they are not useful. I do not understand this. Please explain why requesting features is against policy or not useful.
2016-11-21 13:06 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > On Mon, 21 Nov 2016 12:48:31 +0100 > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >> 2016-11-21 12:04 GMT+01:00 wm4 <nfxjfg@googlemail.com>: >> > On Mon, 21 Nov 2016 07:36:44 +0000 >> >> > Can we get support for .doc and .html too? I think these would be great >> > features. >> >> Maybe such comments are not against our policy (that you requested so >> much), in any case they are not useful. > > I do not understand this. Please explain why requesting features is > against policy or not useful. The development mailing list is not the right place for a feature request (so your email is in fact a policy violation). Apart from the fact that doc support does not seem realistic to me: What would FFmpeg do with the files once it contains a decoder? Carl Eugen
On Mon, 21 Nov 2016 13:08:37 +0100 Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2016-11-21 13:06 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > > On Mon, 21 Nov 2016 12:48:31 +0100 > > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > >> 2016-11-21 12:04 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > >> > On Mon, 21 Nov 2016 07:36:44 +0000 > >> > >> > Can we get support for .doc and .html too? I think these would be great > >> > features. > >> > >> Maybe such comments are not against our policy (that you requested so > >> much), in any case they are not useful. > > > > I do not understand this. Please explain why requesting features is > > against policy or not useful. > > The development mailing list is not the right place for a feature request > (so your email is in fact a policy violation). > > Apart from the fact that doc support does not seem realistic to me: > What would FFmpeg do with the files once it contains a decoder? The same as our teletype decoder does.
Le primidi 1er frimaire, an CCXXV, wm4 a écrit : > I do not understand this. Please explain why requesting features is > against policy or not useful. Please stop your passive-aggressive flame bait.
On 20.11.2016 17:57, Martin Vignali wrote: >>> 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. Never mind, I just didn't parse this sentence correctly. >> >> 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). OK. Alternatively you could already send a separate patch adding the 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. I see. In that case it makes sense to explicitly check this, as the check in ff_set_dimensions is more general and might change in the future. On 20.11.2016 18:32, Martin Vignali wrote: >> However, all the rle samples seem to not decode entirely correct, instead >> > containing blue/white rectangular areas. >> > >> > >> > Sorry, forget to answer to that. > The white/blue rect have been add by me to the sample file, in order to > "enable" the rle compression of photoshop. > So i think, the conversion is correct for RLE compression Thanks for explaining that. Best regards, Andreas
2016-11-22 0:57 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > On 20.11.2016 17:57, Martin Vignali wrote: >>>> + 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. > > I see. In that case it makes sense to explicitly check this, as > the check in ff_set_dimensions is more general and might > change in the future. If an application writes psd files with width or height > 30000 (or, horribile dictu, Adobe changes the limits), we want to decode such images (unless strict strict was set). Carl Eugen
On Tue, 22 Nov 2016 01:21:08 +0100, Carl Eugen Hoyos wrote: > If an application writes psd files with width or height > 30000 (or, > horribile dictu, Adobe changes the limits), we want to decode > such images (unless strict strict was set). We shouldn't be encouraging theoretical random broken crapware that ignores specs. Anything over 30000 should be using PSB anyway. Instead of blindly decoding a garbage example it should require "-strict experimental" to "allow non standardized experimental things".
From 1b85107d53ab4e5748156e8e84cb8fbe26317700 Mon Sep 17 00:00:00 2001 From: Martin Vignali <martin.vignali@gmail.com> Date: Fri, 18 Nov 2016 18:18:36 +0100 Subject: [PATCH 2/2] libavformat : add Photoshop PSD file. --- libavformat/Makefile | 1 + libavformat/allformats.c | 1 + libavformat/img2dec.c | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/libavformat/Makefile b/libavformat/Makefile index f93658d..f803d04 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -235,6 +235,7 @@ OBJS-$(CONFIG_IMAGE_PGM_PIPE_DEMUXER) += img2dec.o img2.o OBJS-$(CONFIG_IMAGE_PICTOR_PIPE_DEMUXER) += img2dec.o img2.o OBJS-$(CONFIG_IMAGE_PNG_PIPE_DEMUXER) += img2dec.o img2.o OBJS-$(CONFIG_IMAGE_PPM_PIPE_DEMUXER) += img2dec.o img2.o +OBJS-$(CONFIG_IMAGE_PSD_PIPE_DEMUXER) += img2dec.o img2.o OBJS-$(CONFIG_IMAGE_QDRAW_PIPE_DEMUXER) += img2dec.o img2.o OBJS-$(CONFIG_IMAGE_SGI_PIPE_DEMUXER) += img2dec.o img2.o OBJS-$(CONFIG_IMAGE_SUNRAST_PIPE_DEMUXER) += img2dec.o img2.o diff --git a/libavformat/allformats.c b/libavformat/allformats.c index 6a216ef..9d77b9c 100644 --- a/libavformat/allformats.c +++ b/libavformat/allformats.c @@ -366,6 +366,7 @@ void av_register_all(void) REGISTER_DEMUXER (IMAGE_PICTOR_PIPE, image_pictor_pipe); REGISTER_DEMUXER (IMAGE_PNG_PIPE, image_png_pipe); REGISTER_DEMUXER (IMAGE_PPM_PIPE, image_ppm_pipe); + REGISTER_DEMUXER (IMAGE_PSD_PIPE, image_psd_pipe); REGISTER_DEMUXER (IMAGE_QDRAW_PIPE, image_qdraw_pipe); REGISTER_DEMUXER (IMAGE_SGI_PIPE, image_sgi_pipe); REGISTER_DEMUXER (IMAGE_SUNRAST_PIPE, image_sunrast_pipe); diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c index a920f46..d668249 100644 --- a/libavformat/img2dec.c +++ b/libavformat/img2dec.c @@ -822,6 +822,39 @@ static int png_probe(AVProbeData *p) return 0; } +static int psd_probe(AVProbeData *p) +{ + const uint8_t *b = p->buf; + int ret = 0; + uint16_t color_mode; + + if (AV_RL32(b) == MKTAG('8','B','P','S')) { + ret += 1; + } else { + return 0; + } + + if ((b[4] == 0) && (b[5] == 1)) {/* version 1 is PSD, version 2 is PSB */ + ret += 1; + } else { + return 0; + } + + if ((AV_RL32(b+6) == 0) && (AV_RL16(b+10) == 0))/* reserved must be 0 */ + ret += 1; + if (p->buf_size < 26) + return 0; + + color_mode = AV_RB16(b+24); + if ((color_mode <= 9) && (color_mode != 5) && (color_mode != 6)) + ret += 1; + + if (ret) + return AVPROBE_SCORE_EXTENSION + ret; + + return 0; +} + static int sgi_probe(AVProbeData *p) { const uint8_t *b = p->buf; @@ -947,6 +980,7 @@ IMAGEAUTO_DEMUXER(pgmyuv, AV_CODEC_ID_PGMYUV) IMAGEAUTO_DEMUXER(pictor, AV_CODEC_ID_PICTOR) IMAGEAUTO_DEMUXER(png, AV_CODEC_ID_PNG) IMAGEAUTO_DEMUXER(ppm, AV_CODEC_ID_PPM) +IMAGEAUTO_DEMUXER(psd, AV_CODEC_ID_PSD) IMAGEAUTO_DEMUXER(qdraw, AV_CODEC_ID_QDRAW) IMAGEAUTO_DEMUXER(sgi, AV_CODEC_ID_SGI) IMAGEAUTO_DEMUXER(sunrast, AV_CODEC_ID_SUNRAST) -- 1.9.3 (Apple Git-50)