Commit 478f1c3d authored by Ronald S. Bultje's avatar Ronald S. Bultje

png: split header state and data state in two separate variables.

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)
parent 1f50baa2
...@@ -42,11 +42,6 @@ ...@@ -42,11 +42,6 @@
#define PNG_FILTER_VALUE_PAETH 4 #define PNG_FILTER_VALUE_PAETH 4
#define PNG_FILTER_VALUE_MIXED 5 #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 NB_PASSES 7
#define PNGSIG 0x89504e470d0a1a0a #define PNGSIG 0x89504e470d0a1a0a
......
...@@ -36,6 +36,16 @@ ...@@ -36,6 +36,16 @@
#include <zlib.h> #include <zlib.h>
enum PNGHeaderState {
PNG_IHDR = 1 << 0,
PNG_PLTE = 1 << 1,
};
enum PNGImageState {
PNG_IDAT = 1 << 0,
PNG_ALLIMAGE = 1 << 1,
};
typedef struct PNGDecContext { typedef struct PNGDecContext {
PNGDSPContext dsp; PNGDSPContext dsp;
AVCodecContext *avctx; AVCodecContext *avctx;
...@@ -45,7 +55,8 @@ typedef struct PNGDecContext { ...@@ -45,7 +55,8 @@ typedef struct PNGDecContext {
ThreadFrame last_picture; ThreadFrame last_picture;
ThreadFrame picture; ThreadFrame picture;
int state; enum PNGHeaderState hdr_state;
enum PNGImageState pic_state;
int width, height; int width, height;
int cur_w, cur_h; int cur_w, cur_h;
int last_w, last_h; int last_w, last_h;
...@@ -334,7 +345,7 @@ static void png_handle_row(PNGDecContext *s) ...@@ -334,7 +345,7 @@ static void png_handle_row(PNGDecContext *s)
} }
s->y++; s->y++;
if (s->y == s->cur_h) { 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->filter_type == PNG_FILTER_TYPE_LOCO) {
if (s->bit_depth == 16) { if (s->bit_depth == 16) {
deloco_rgb16((uint16_t *)ptr, s->row_size / 2, deloco_rgb16((uint16_t *)ptr, s->row_size / 2,
...@@ -369,7 +380,7 @@ static void png_handle_row(PNGDecContext *s) ...@@ -369,7 +380,7 @@ static void png_handle_row(PNGDecContext *s)
memset(s->last_row, 0, s->row_size); memset(s->last_row, 0, s->row_size);
for (;;) { for (;;) {
if (s->pass == NB_PASSES - 1) { if (s->pass == NB_PASSES - 1) {
s->state |= PNG_ALLIMAGE; s->pic_state |= PNG_ALLIMAGE;
goto the_end; goto the_end;
} else { } else {
s->pass++; s->pass++;
...@@ -404,7 +415,7 @@ static int png_decode_idat(PNGDecContext *s, int length) ...@@ -404,7 +415,7 @@ static int png_decode_idat(PNGDecContext *s, int length)
return AVERROR_EXTERNAL; return AVERROR_EXTERNAL;
} }
if (s->zstream.avail_out == 0) { if (s->zstream.avail_out == 0) {
if (!(s->state & PNG_ALLIMAGE)) { if (!(s->pic_state & PNG_ALLIMAGE)) {
png_handle_row(s); png_handle_row(s);
} }
s->zstream.avail_out = s->crow_size; s->zstream.avail_out = s->crow_size;
...@@ -541,12 +552,12 @@ static int decode_ihdr_chunk(AVCodecContext *avctx, PNGDecContext *s, ...@@ -541,12 +552,12 @@ static int decode_ihdr_chunk(AVCodecContext *avctx, PNGDecContext *s,
if (length != 13) if (length != 13)
return AVERROR_INVALIDDATA; return AVERROR_INVALIDDATA;
if (s->state & PNG_IDAT) { if (s->pic_state & PNG_IDAT) {
av_log(avctx, AV_LOG_ERROR, "IHDR after IDAT\n"); av_log(avctx, AV_LOG_ERROR, "IHDR after IDAT\n");
return AVERROR_INVALIDDATA; return AVERROR_INVALIDDATA;
} }
if (s->state & PNG_IHDR) { if (s->hdr_state & PNG_IHDR) {
av_log(avctx, AV_LOG_ERROR, "Multiple IHDR\n"); av_log(avctx, AV_LOG_ERROR, "Multiple IHDR\n");
return AVERROR_INVALIDDATA; return AVERROR_INVALIDDATA;
} }
...@@ -569,7 +580,7 @@ static int decode_ihdr_chunk(AVCodecContext *avctx, PNGDecContext *s, ...@@ -569,7 +580,7 @@ static int decode_ihdr_chunk(AVCodecContext *avctx, PNGDecContext *s,
s->filter_type = bytestream2_get_byte(&s->gb); s->filter_type = bytestream2_get_byte(&s->gb);
s->interlace_type = bytestream2_get_byte(&s->gb); s->interlace_type = bytestream2_get_byte(&s->gb);
bytestream2_skip(&s->gb, 4); /* crc */ bytestream2_skip(&s->gb, 4); /* crc */
s->state |= PNG_IHDR; s->hdr_state |= PNG_IHDR;
if (avctx->debug & FF_DEBUG_PICT_INFO) if (avctx->debug & FF_DEBUG_PICT_INFO)
av_log(avctx, AV_LOG_DEBUG, "width=%d height=%d depth=%d color_type=%d " 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", "compression_type=%d filter_type=%d interlace_type=%d\n",
...@@ -585,7 +596,7 @@ error: ...@@ -585,7 +596,7 @@ error:
static int decode_phys_chunk(AVCodecContext *avctx, PNGDecContext *s) 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"); av_log(avctx, AV_LOG_ERROR, "pHYs after IDAT\n");
return AVERROR_INVALIDDATA; return AVERROR_INVALIDDATA;
} }
...@@ -605,11 +616,11 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s, ...@@ -605,11 +616,11 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s,
int ret; int ret;
size_t byte_depth = s->bit_depth > 8 ? 2 : 1; 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"); av_log(avctx, AV_LOG_ERROR, "IDAT without IHDR\n");
return AVERROR_INVALIDDATA; return AVERROR_INVALIDDATA;
} }
if (!(s->state & PNG_IDAT)) { if (!(s->pic_state & PNG_IDAT)) {
/* init image info */ /* init image info */
avctx->width = s->width; avctx->width = s->width;
avctx->height = s->height; avctx->height = s->height;
...@@ -734,7 +745,7 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s, ...@@ -734,7 +745,7 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s,
s->zstream.next_out = s->crow_buf; s->zstream.next_out = s->crow_buf;
} }
s->state |= PNG_IDAT; s->pic_state |= PNG_IDAT;
/* set image to non-transparent bpp while decompressing */ /* set image to non-transparent bpp while decompressing */
if (s->has_trns && s->color_type != PNG_COLOR_TYPE_PALETTE) if (s->has_trns && s->color_type != PNG_COLOR_TYPE_PALETTE)
...@@ -770,7 +781,7 @@ static int decode_plte_chunk(AVCodecContext *avctx, PNGDecContext *s, ...@@ -770,7 +781,7 @@ static int decode_plte_chunk(AVCodecContext *avctx, PNGDecContext *s,
} }
for (; i < 256; i++) for (; i < 256; i++)
s->palette[i] = (0xFFU << 24); s->palette[i] = (0xFFU << 24);
s->state |= PNG_PLTE; s->hdr_state |= PNG_PLTE;
bytestream2_skip(&s->gb, 4); /* crc */ bytestream2_skip(&s->gb, 4); /* crc */
return 0; return 0;
...@@ -781,18 +792,18 @@ static int decode_trns_chunk(AVCodecContext *avctx, PNGDecContext *s, ...@@ -781,18 +792,18 @@ static int decode_trns_chunk(AVCodecContext *avctx, PNGDecContext *s,
{ {
int v, i; int v, i;
if (!(s->state & PNG_IHDR)) { if (!(s->hdr_state & PNG_IHDR)) {
av_log(avctx, AV_LOG_ERROR, "trns before IHDR\n"); av_log(avctx, AV_LOG_ERROR, "trns before IHDR\n");
return AVERROR_INVALIDDATA; return AVERROR_INVALIDDATA;
} }
if (s->state & PNG_IDAT) { if (s->pic_state & PNG_IDAT) {
av_log(avctx, AV_LOG_ERROR, "trns after IDAT\n"); av_log(avctx, AV_LOG_ERROR, "trns after IDAT\n");
return AVERROR_INVALIDDATA; return AVERROR_INVALIDDATA;
} }
if (s->color_type == PNG_COLOR_TYPE_PALETTE) { 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; return AVERROR_INVALIDDATA;
for (i = 0; i < length; i++) { for (i = 0; i < length; i++) {
...@@ -906,7 +917,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s, ...@@ -906,7 +917,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s,
if (length != 26) if (length != 26)
return AVERROR_INVALIDDATA; return AVERROR_INVALIDDATA;
if (!(s->state & PNG_IHDR)) { if (!(s->hdr_state & PNG_IHDR)) {
av_log(avctx, AV_LOG_ERROR, "fctl before IHDR\n"); av_log(avctx, AV_LOG_ERROR, "fctl before IHDR\n");
return AVERROR_INVALIDDATA; return AVERROR_INVALIDDATA;
} }
...@@ -1122,13 +1133,13 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s, ...@@ -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 (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; return 0;
else else
goto exit_loop; goto exit_loop;
} }
av_log(avctx, AV_LOG_ERROR, "%d bytes left\n", length); 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) && avctx->strict_std_compliance <= FF_COMPLIANCE_NORMAL)
goto exit_loop; goto exit_loop;
ret = AVERROR_INVALIDDATA; ret = AVERROR_INVALIDDATA;
...@@ -1228,9 +1239,9 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s, ...@@ -1228,9 +1239,9 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s,
break; break;
} }
case MKTAG('I', 'E', 'N', 'D'): 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"); 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; ret = AVERROR_INVALIDDATA;
goto fail; goto fail;
} }
...@@ -1330,7 +1341,9 @@ static int decode_frame_png(AVCodecContext *avctx, ...@@ -1330,7 +1341,9 @@ static int decode_frame_png(AVCodecContext *avctx,
return AVERROR_INVALIDDATA; 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 */ /* init the zlib */
s->zstream.zalloc = ff_png_zalloc; s->zstream.zalloc = ff_png_zalloc;
...@@ -1377,7 +1390,7 @@ static int decode_frame_apng(AVCodecContext *avctx, ...@@ -1377,7 +1390,7 @@ static int decode_frame_apng(AVCodecContext *avctx,
FFSWAP(ThreadFrame, s->picture, s->last_picture); FFSWAP(ThreadFrame, s->picture, s->last_picture);
p = s->picture.f; p = s->picture.f;
if (!(s->state & PNG_IHDR)) { if (!(s->hdr_state & PNG_IHDR)) {
if (!avctx->extradata_size) if (!avctx->extradata_size)
return AVERROR_INVALIDDATA; return AVERROR_INVALIDDATA;
...@@ -1397,14 +1410,14 @@ static int decode_frame_apng(AVCodecContext *avctx, ...@@ -1397,14 +1410,14 @@ static int decode_frame_apng(AVCodecContext *avctx,
goto end; goto end;
} }
s->y = 0; s->y = 0;
s->state &= ~(PNG_IDAT | PNG_ALLIMAGE); s->pic_state = 0;
bytestream2_init(&s->gb, avpkt->data, avpkt->size); bytestream2_init(&s->gb, avpkt->data, avpkt->size);
if ((ret = decode_frame_common(avctx, s, p, avpkt)) < 0) if ((ret = decode_frame_common(avctx, s, p, avpkt)) < 0)
goto end; 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"); 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; ret = AVERROR_INVALIDDATA;
goto end; goto end;
} }
...@@ -1453,7 +1466,7 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src) ...@@ -1453,7 +1466,7 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src)
memcpy(pdst->palette, psrc->palette, sizeof(pdst->palette)); 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); ff_thread_release_buffer(dst, &pdst->last_picture);
if (psrc->last_picture.f->data[0] && if (psrc->last_picture.f->data[0] &&
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment