[FFmpeg-devel] libavcodec : add psd image file decoder
Martin Vignali
martin.vignali at gmail.com
Mon Jul 25 10:57:22 EEST 2016
2016-07-25 2:46 GMT+02:00 Michael Niedermayer <michael at niedermayer.cc>:
> On Sun, Jul 24, 2016 at 03:21:00PM +0200, Martin Vignali wrote:
> > > breaks fate
> > >
> > > make: *** [fate-filter-metadata-ebur128] Error 1
> > > --- - 2016-07-23 18:50:11.633752058 +0200
> > > +++ tests/data/fate/probe-format-roundup1414 2016-07-23
> > > 18:50:10.913635588 +0200
> > > @@ -1 +1 @@
> > > -mpeg
> > > +psd_pipe
> > > Test probe-format-roundup1414 failed. Look at
> > > tests/data/fate/probe-format-roundup1414.err for details.
> > > make: *** [fate-probe-format-roundup1414] Error 1
> > > --- - 2016-07-23 18:50:11.657384876 +0200
> > > +++ tests/data/fate/probe-format-roundup997 2016-07-23
> > > 18:50:10.917635588 +0200
> > > @@ -1 +1 @@
> > > -mpeg
> > > +psd_pipe
> > > Test probe-format-roundup997 failed. Look at
> > > tests/data/fate/probe-format-roundup997.err for details.
> > > make: *** [fate-probe-format-roundup997] Error 1
> > > --- - 2016-07-23 18:50:11.667958765 +0200
> > > +++ tests/data/fate/probe-format-roundup1383 2016-07-23
> > > 18:50:10.917635588 +0200
> > > @@ -1 +1 @@
> > > -mp3
> > > +psd_pipe
> > > Test probe-format-roundup1383 failed. Look at
> > > tests/data/fate/probe-format-roundup1383.err for details.
> > > make: *** [fate-probe-format-roundup1383] Error 1
> > > make: Target `fate' not remade because of errors.
> > >
> > >
> > Hello,
> >
> > Corrected patch in attach who fix the psd_probe function.
> >
> > Comments welcome
> >
> > Martin
>
> > Changelog | 1
> > doc/general.texi | 2
> > libavcodec/Makefile | 1
> > libavcodec/allcodecs.c | 1
> > libavcodec/avcodec.h | 1
> > libavcodec/codec_desc.c | 7
> > libavcodec/psd.c | 395
> +++++++++++++++++++++++++++++++++++++++++++++++
> > libavformat/Makefile | 1
> > libavformat/allformats.c | 1
> > libavformat/img2.c | 1
> > libavformat/img2dec.c | 35 ++++
> > 11 files changed, 446 insertions(+)
>
> this should be split into 2 patches one for libavcodec and libavformat
>
>
> [...]
> > +static int decode_rle(PSDContext * s){
> > + unsigned int scanline_count;
> > + unsigned int sl, count;
> > + unsigned long target_index = 0;
> > +
> > + scanline_count = s->height * s->channel_count;
> > +
> > + /* scanline table */
> > + if (bytestream2_get_bytes_left(&s->gb) < scanline_count * 2) {
> > + av_log(s->avctx, AV_LOG_ERROR, "Not enough data for rle
> scanline table.\n");
> > + return AVERROR_INVALIDDATA;
> > + }
> > + bytestream2_skip(&s->gb, scanline_count * 2);/* size of each
> scanline */
> > +
> > + /* decode rle data scanline by scanline */
> > + for (sl = 0; sl < scanline_count; sl++) {
> > + count = 0;
> > +
> > + while (count < s->line_size) {
>
> > + char rle_char = bytestream2_get_byte(&s->gb);
>
> please use intXY_t or int, char can be unsigned
>
>
> > +
> > + if (rle_char <= 0) {/* byte repeat */
>
> > + rle_char *= -1;
>
> this is not safe
> rle_char is just 8 bit -128 cannot be represented as positive value
>
>
> > +
> > + if (bytestream2_get_bytes_left(&s->gb) < 1) {
> > + av_log(s->avctx, AV_LOG_ERROR, "Not enough data for
> rle scanline.\n");
> > + return AVERROR_INVALIDDATA;
> > + }
> > +
> > + if (target_index + rle_char > s->uncompressed_size) {
> > + av_log(s->avctx, AV_LOG_ERROR, "Invalid rle
> char.\n");
> > + return AVERROR_INVALIDDATA;
> > + }
> > +
>
> > + uint8_t v = bytestream2_get_byte(&s->gb);
>
> mixed declarations and code, please move the uint8_t v up
>
>
> > + for (unsigned char p = 0; p <= rle_char; p++) {
> > + s->tmp[target_index++] = v;
> > + }
> > + } else {
> > + if (bytestream2_get_bytes_left(&s->gb) < rle_char) {
> > + av_log(s->avctx, AV_LOG_ERROR, "Not enough data for
> rle scanline.\n");
> > + return AVERROR_INVALIDDATA;
> > + }
> > +
>
> > + if (target_index + rle_char > s->uncompressed_size) {
> > + av_log(s->avctx, AV_LOG_ERROR, "Invalid rle
> char.\n");
> > + return AVERROR_INVALIDDATA;
> > + }
>
> isnt this off by 1?
> rle_char = 0
> target_index == uncompressed_size would pass but write at
> uncompressed_size whch would be after the array if iam not mistaken
>
>
> > +
> > + for (int p = 0; p <= rle_char; p++) {
> > + uint8_t v = bytestream2_get_byte(&s->gb);
> > + s->tmp[target_index++] = v;
> > + }
> > + }
> > + count += rle_char + 1;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int decode_frame(AVCodecContext *avctx, void *data,
> > + int *got_frame, AVPacket *avpkt)
> > +{
> > + int ret;
> > + uint8_t *ptr;
> > + const uint8_t * ptr_data;
> > + int index_out, c, y, x, p;
> > +
> > + 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_RGB24;
> > + } else if (s->channel_depth == 16) {
> > + avctx->pix_fmt = AV_PIX_FMT_RGB48BE;
> > + } else {
> > + avpriv_report_missing_feature(avctx, "channel depth
> unsupported for rgb %d", s->channel_depth);
> > + return AVERROR_PATCHWELCOME;
> > + }
> > + } else if (s->channel_count == 4) {
> > + if (s->channel_depth == 8) {
> > + avctx->pix_fmt = AV_PIX_FMT_RGBA;
> > + } else if (s->channel_depth == 16) {
> > + avctx->pix_fmt = AV_PIX_FMT_RGBA64BE;
> > + } else {
> > + avpriv_report_missing_feature(avctx, "channel depth
> unsupported for rgb %d", s->channel_depth);
> > + return AVERROR_PATCHWELCOME;
> > + }
> > + } else {
> > + avpriv_report_missing_feature(avctx, "channel count
> unsupported for rgb %d", s->channel_count);
> > + return AVERROR_PATCHWELCOME;
> > + }
> > + break;
> > + case PSD_GRAYSCALE:
> > + if (s->channel_count == 1) {
> > + if (s->channel_depth == 8) {
> > + avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> > + } else if (s->channel_depth == 16) {
> > + avctx->pix_fmt = AV_PIX_FMT_GRAY16BE;
> > + } else {
> > + avpriv_report_missing_feature(avctx, "channel depth
> unsupported for grayscale %d", s->channel_depth);
> > + return AVERROR_PATCHWELCOME;
> > + }
> > + } else if (s->channel_count == 2) {
> > + if (s->channel_depth == 8) {
> > + avctx->pix_fmt = AV_PIX_FMT_YA8;
> > + } else if (s->channel_depth == 16) {
> > + avctx->pix_fmt = AV_PIX_FMT_YA16BE;
> > + } else {
> > + avpriv_report_missing_feature(avctx, "channel depth
> unsupported for grayscale %d", s->channel_depth);
> > + return AVERROR_PATCHWELCOME;
> > + }
> > + } else {
> > + avpriv_report_missing_feature(avctx, "channel count
> unsupported for grayscale %d", s->channel_count);
> > + return AVERROR_PATCHWELCOME;
> > + }
> > + break;
> > + default:
> > + avpriv_report_missing_feature(avctx, "color mode unsupported
> %d", s->color_mode);
> > + return AVERROR_PATCHWELCOME;
> > + }
> > +
> > + if ((ret = ff_get_buffer(avctx, picture, 0)) < 0)
> > + return ret;
> > +
>
> > + /* decode picture if need */
> > + if (s->compression == PSD_RLE) {
> > + s->tmp = av_malloc(s->uncompressed_size);
>
> missing malloc failure check
>
> thx
>
>
> Thanks for the review
New patch in attach.
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libavformat-add-Photoshop-PSD-file.patch
Type: text/x-patch
Size: 3141 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160725/c57fbd75/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-libavcodec-add-decoder-for-Photoshop-PSD-image-file.patch
Type: text/x-patch
Size: 16386 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160725/c57fbd75/attachment-0001.bin>
More information about the ffmpeg-devel
mailing list