From patchwork Mon Apr 3 14:09:30 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Ronald S. Bultje" X-Patchwork-Id: 3268 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.44.195 with SMTP id s186csp103241vss; Mon, 3 Apr 2017 07:16:54 -0700 (PDT) X-Received: by 10.28.87.138 with SMTP id l132mr9969337wmb.95.1491229014402; Mon, 03 Apr 2017 07:16:54 -0700 (PDT) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id p12si20152301wrc.129.2017.04.03.07.16.53; Mon, 03 Apr 2017 07:16:54 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@gmail.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=gmail.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1EF3E688342; Mon, 3 Apr 2017 17:16:49 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-qk0-f171.google.com (mail-qk0-f171.google.com [209.85.220.171]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 22B7C6882A8 for ; Mon, 3 Apr 2017 17:16:42 +0300 (EEST) Received: by mail-qk0-f171.google.com with SMTP id p22so115198529qka.3 for ; Mon, 03 Apr 2017 07:16:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=Aft3nZWxO4+RUew/QzwVfVaFu65ubY0eQhQRzAxbH/8=; b=P5vfewz5ZCoaYiPILB8D407rr0eZkghApuDAWdbzmgfpa5O5FSarrzOpKeVxr8Y1I2 m+B/Kzf+mSETlMgfRor51P6pzDnF7ig1oOTId/f/wBnXQS+uFHmhTNdmtrkljl7T58mI YhCffWFhUZdbxIXeMfsh/JkRGBaM2CY6C1M7s2CHSAMlJP4O+ihifwivjLaCqEDOb1Dz CXFhLVF5F2Mdxn2DARNMNLi8G4qq/olMQ2mNI1GEmebIBdK28Ic+K4eGQ+2/fCpRPpgQ E+pQ8vw1rDI3oWnYB0I1ytGzqyoedCDvVkGqM6BqEHVK45pcW17D2ToBYQrsi/8VNZoU iWoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=Aft3nZWxO4+RUew/QzwVfVaFu65ubY0eQhQRzAxbH/8=; b=oC/GYGrBjJeBXBYSZNutbOh8W6Ys1sHJ0Tfo0+X0jiF38p1shdYtT0/+IPooPjMYkP 8bNd9gYLDvnedQLvTHkmwXfNABlJ6dYbvTs7e3wxqMkKU3SvGg1jh0LOLPYf526PcIXj qBr7lQtrQcJjFW5NGvk15OMaI652cKwz4Zxxi+bIZp5aTNB0iWexs8eDoN5vGy3NSklJ y4U9YQAKLnV9/56Ri8OsZTylP8y0BuArzujMo9v1rGS/dXL0ttFRJmGcM5f1EWzeO+AK lhUoW6GAWjeRhRd1/eJCurLMBTrMxiDZnjjn3uKpZCe/c6KB/uJGCkDVIAUBZJFoiGLs 1pqw== X-Gm-Message-State: AFeK/H2PEWS57PiJCY+EcZDZZxR4HqRfhZRREFsJSr8SXsI7EO5hc6ypHX0bOkhXswJDRw== X-Received: by 10.55.67.66 with SMTP id q63mr15270855qka.49.1491228572930; Mon, 03 Apr 2017 07:09:32 -0700 (PDT) Received: from localhost.localdomain ([65.206.95.146]) by smtp.gmail.com with ESMTPSA id g185sm9656668qkc.7.2017.04.03.07.09.32 (version=TLS1 cipher=AES128-SHA bits=128/128); Mon, 03 Apr 2017 07:09:32 -0700 (PDT) From: "Ronald S. Bultje" To: ffmpeg-devel@ffmpeg.org Date: Mon, 3 Apr 2017 10:09:30 -0400 Message-Id: <1491228570-19107-1-git-send-email-rsbultje@gmail.com> X-Mailer: git-send-email 2.8.1 In-Reply-To: <20170331225340.GF4714@nb4> References: <20170331225340.GF4714@nb4> Subject: [FFmpeg-devel] [PATCH] png: split header state and data state in two separate variables. X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: "Ronald S. Bultje" MIME-Version: 1.0 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Fixes a reported (but false) race condition in tsan for fate-apng: WARNING: ThreadSanitizer: data race (pid=6274) Read of size 4 at 0x7d680001ec78 by main thread (mutexes: write M1338): #0 update_thread_context src/libavcodec/pngdec.c:1456 (ffmpeg+0x000000dacf0c) [..] Previous write of size 4 at 0x7d680001ec78 by thread T1 (mutexes: write M1335): #0 decode_idat_chunk src/libavcodec/pngdec.c:737 (ffmpeg+0x000000dae951) --- libavcodec/png.h | 5 ----- libavcodec/pngdec.c | 65 ++++++++++++++++++++++++++++++++--------------------- 2 files changed, 39 insertions(+), 31 deletions(-) diff --git a/libavcodec/png.h b/libavcodec/png.h index 948c2f7..e967fcf 100644 --- a/libavcodec/png.h +++ b/libavcodec/png.h @@ -42,11 +42,6 @@ #define PNG_FILTER_VALUE_PAETH 4 #define PNG_FILTER_VALUE_MIXED 5 -#define PNG_IHDR 0x0001 -#define PNG_IDAT 0x0002 -#define PNG_ALLIMAGE 0x0004 -#define PNG_PLTE 0x0008 - #define NB_PASSES 7 #define PNGSIG 0x89504e470d0a1a0a diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c index c08665b..d184c34 100644 --- a/libavcodec/pngdec.c +++ b/libavcodec/pngdec.c @@ -36,6 +36,16 @@ #include +enum PNGHeaderState { + PNG_IHDR = 1 << 0, + PNG_PLTE = 1 << 1, +}; + +enum PNGImageState { + PNG_IDAT = 1 << 0, + PNG_ALLIMAGE = 1 << 1, +}; + typedef struct PNGDecContext { PNGDSPContext dsp; AVCodecContext *avctx; @@ -45,7 +55,8 @@ typedef struct PNGDecContext { ThreadFrame last_picture; ThreadFrame picture; - int state; + enum PNGHeaderState hdr_state; + enum PNGImageState pic_state; int width, height; int cur_w, cur_h; int last_w, last_h; @@ -334,7 +345,7 @@ static void png_handle_row(PNGDecContext *s) } s->y++; if (s->y == s->cur_h) { - s->state |= PNG_ALLIMAGE; + s->pic_state |= PNG_ALLIMAGE; if (s->filter_type == PNG_FILTER_TYPE_LOCO) { if (s->bit_depth == 16) { deloco_rgb16((uint16_t *)ptr, s->row_size / 2, @@ -369,7 +380,7 @@ static void png_handle_row(PNGDecContext *s) memset(s->last_row, 0, s->row_size); for (;;) { if (s->pass == NB_PASSES - 1) { - s->state |= PNG_ALLIMAGE; + s->pic_state |= PNG_ALLIMAGE; goto the_end; } else { s->pass++; @@ -404,7 +415,7 @@ static int png_decode_idat(PNGDecContext *s, int length) return AVERROR_EXTERNAL; } if (s->zstream.avail_out == 0) { - if (!(s->state & PNG_ALLIMAGE)) { + if (!(s->pic_state & PNG_ALLIMAGE)) { png_handle_row(s); } s->zstream.avail_out = s->crow_size; @@ -541,12 +552,12 @@ static int decode_ihdr_chunk(AVCodecContext *avctx, PNGDecContext *s, if (length != 13) return AVERROR_INVALIDDATA; - if (s->state & PNG_IDAT) { + if (s->pic_state & PNG_IDAT) { av_log(avctx, AV_LOG_ERROR, "IHDR after IDAT\n"); return AVERROR_INVALIDDATA; } - if (s->state & PNG_IHDR) { + if (s->hdr_state & PNG_IHDR) { av_log(avctx, AV_LOG_ERROR, "Multiple IHDR\n"); return AVERROR_INVALIDDATA; } @@ -569,7 +580,7 @@ static int decode_ihdr_chunk(AVCodecContext *avctx, PNGDecContext *s, s->filter_type = bytestream2_get_byte(&s->gb); s->interlace_type = bytestream2_get_byte(&s->gb); bytestream2_skip(&s->gb, 4); /* crc */ - s->state |= PNG_IHDR; + s->hdr_state |= PNG_IHDR; if (avctx->debug & FF_DEBUG_PICT_INFO) av_log(avctx, AV_LOG_DEBUG, "width=%d height=%d depth=%d color_type=%d " "compression_type=%d filter_type=%d interlace_type=%d\n", @@ -585,7 +596,7 @@ error: static int decode_phys_chunk(AVCodecContext *avctx, PNGDecContext *s) { - if (s->state & PNG_IDAT) { + if (s->pic_state & PNG_IDAT) { av_log(avctx, AV_LOG_ERROR, "pHYs after IDAT\n"); return AVERROR_INVALIDDATA; } @@ -605,11 +616,11 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s, int ret; size_t byte_depth = s->bit_depth > 8 ? 2 : 1; - if (!(s->state & PNG_IHDR)) { + if (!(s->hdr_state & PNG_IHDR)) { av_log(avctx, AV_LOG_ERROR, "IDAT without IHDR\n"); return AVERROR_INVALIDDATA; } - if (!(s->state & PNG_IDAT)) { + if (!(s->pic_state & PNG_IDAT)) { /* init image info */ avctx->width = s->width; avctx->height = s->height; @@ -734,7 +745,7 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s, s->zstream.next_out = s->crow_buf; } - s->state |= PNG_IDAT; + s->pic_state |= PNG_IDAT; /* set image to non-transparent bpp while decompressing */ if (s->has_trns && s->color_type != PNG_COLOR_TYPE_PALETTE) @@ -770,7 +781,7 @@ static int decode_plte_chunk(AVCodecContext *avctx, PNGDecContext *s, } for (; i < 256; i++) s->palette[i] = (0xFFU << 24); - s->state |= PNG_PLTE; + s->hdr_state |= PNG_PLTE; bytestream2_skip(&s->gb, 4); /* crc */ return 0; @@ -781,18 +792,18 @@ static int decode_trns_chunk(AVCodecContext *avctx, PNGDecContext *s, { int v, i; - if (!(s->state & PNG_IHDR)) { + if (!(s->hdr_state & PNG_IHDR)) { av_log(avctx, AV_LOG_ERROR, "trns before IHDR\n"); return AVERROR_INVALIDDATA; } - if (s->state & PNG_IDAT) { + if (s->pic_state & PNG_IDAT) { av_log(avctx, AV_LOG_ERROR, "trns after IDAT\n"); return AVERROR_INVALIDDATA; } if (s->color_type == PNG_COLOR_TYPE_PALETTE) { - if (length > 256 || !(s->state & PNG_PLTE)) + if (length > 256 || !(s->hdr_state & PNG_PLTE)) return AVERROR_INVALIDDATA; for (i = 0; i < length; i++) { @@ -906,7 +917,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s, if (length != 26) return AVERROR_INVALIDDATA; - if (!(s->state & PNG_IHDR)) { + if (!(s->hdr_state & PNG_IHDR)) { av_log(avctx, AV_LOG_ERROR, "fctl before IHDR\n"); return AVERROR_INVALIDDATA; } @@ -1122,13 +1133,13 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s, } if (CONFIG_APNG_DECODER && avctx->codec_id == AV_CODEC_ID_APNG && length == 0) { - if (!(s->state & PNG_IDAT)) + if (!(s->pic_state & PNG_IDAT)) return 0; else goto exit_loop; } av_log(avctx, AV_LOG_ERROR, "%d bytes left\n", length); - if ( s->state & PNG_ALLIMAGE + if ( s->pic_state & PNG_ALLIMAGE && avctx->strict_std_compliance <= FF_COMPLIANCE_NORMAL) goto exit_loop; ret = AVERROR_INVALIDDATA; @@ -1228,9 +1239,9 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s, break; } case MKTAG('I', 'E', 'N', 'D'): - if (!(s->state & PNG_ALLIMAGE)) + if (!(s->pic_state & PNG_ALLIMAGE)) av_log(avctx, AV_LOG_ERROR, "IEND without all image\n"); - if (!(s->state & (PNG_ALLIMAGE|PNG_IDAT))) { + if (!(s->pic_state & (PNG_ALLIMAGE|PNG_IDAT))) { ret = AVERROR_INVALIDDATA; goto fail; } @@ -1330,7 +1341,9 @@ static int decode_frame_png(AVCodecContext *avctx, return AVERROR_INVALIDDATA; } - s->y = s->state = s->has_trns = 0; + s->y = s->has_trns = 0; + s->hdr_state = 0; + s->pic_state = 0; /* init the zlib */ s->zstream.zalloc = ff_png_zalloc; @@ -1377,7 +1390,7 @@ static int decode_frame_apng(AVCodecContext *avctx, FFSWAP(ThreadFrame, s->picture, s->last_picture); p = s->picture.f; - if (!(s->state & PNG_IHDR)) { + if (!(s->hdr_state & PNG_IHDR)) { if (!avctx->extradata_size) return AVERROR_INVALIDDATA; @@ -1397,14 +1410,14 @@ static int decode_frame_apng(AVCodecContext *avctx, goto end; } s->y = 0; - s->state &= ~(PNG_IDAT | PNG_ALLIMAGE); + s->pic_state = 0; bytestream2_init(&s->gb, avpkt->data, avpkt->size); if ((ret = decode_frame_common(avctx, s, p, avpkt)) < 0) goto end; - if (!(s->state & PNG_ALLIMAGE)) + if (!(s->pic_state & PNG_ALLIMAGE)) av_log(avctx, AV_LOG_WARNING, "Frame did not contain a complete image\n"); - if (!(s->state & (PNG_ALLIMAGE|PNG_IDAT))) { + if (!(s->pic_state & (PNG_ALLIMAGE|PNG_IDAT))) { ret = AVERROR_INVALIDDATA; goto end; } @@ -1453,7 +1466,7 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src) memcpy(pdst->palette, psrc->palette, sizeof(pdst->palette)); - pdst->state |= psrc->state & (PNG_IHDR | PNG_PLTE); + pdst->hdr_state |= psrc->hdr_state; ff_thread_release_buffer(dst, &pdst->last_picture); if (psrc->last_picture.f->data[0] &&