| Message ID | 99563f2a-b72a-398d-b9ee-b2f9004bc52d@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Oct 13, 2016 at 10:25 PM, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > If x is 0, 'x - 1' is in the previous line, or worse outside the buffer > for the first line. > > If y is 0, 'x - image->comps[compno].w' is outside the buffer. > I'm slightly puzzled, as you say, these are for edge handling, edges in this case are from the image width to buffer width, and image height to buffer height, respectively So for x or y to be zero, we would need an image thats zero width, or zero height, so the edge starts at zero? How does that happen, and wouldn't it be much simpler to catch that case earlier in the chain and simply error out? A image with either zero width or zero height surely is not something you can encode either way. - Hendrik
On 14.10.2016 00:00, Hendrik Leppkes wrote: > On Thu, Oct 13, 2016 at 10:25 PM, Andreas Cadhalpun > <andreas.cadhalpun@googlemail.com> wrote: >> If x is 0, 'x - 1' is in the previous line, or worse outside the buffer >> for the first line. >> >> If y is 0, 'x - image->comps[compno].w' is outside the buffer. >> > > I'm slightly puzzled, as you say, these are for edge handling, edges > in this case are from the image width to buffer width, and image > height to buffer height, respectively > So for x or y to be zero, we would need an image thats zero width, or > zero height, so the edge starts at zero? > > How does that happen, and wouldn't it be much simpler to catch that > case earlier in the chain and simply error out? A image with either > zero width or zero height surely is not something you can encode > either way. The avctx->width/avctx->height is not zero, but libopenjpeg_copy_unpacked8 does: width = avctx->width / image->comps[compno].dx; height = avctx->height / image->comps[compno].dy; So if e.g. avctx->height is 1 and image->comps[compno].dy is 2, height becomes 0. I'm not sure if that's invalid. I think the other functions are currently not affected by this, but I added the check anyway for robustness. (The cast to int is needed in any case.) Best regards, Andreas
On Fri, Oct 14, 2016 at 12:23:02AM +0200, Andreas Cadhalpun wrote: > On 14.10.2016 00:00, Hendrik Leppkes wrote: > > On Thu, Oct 13, 2016 at 10:25 PM, Andreas Cadhalpun > > <andreas.cadhalpun@googlemail.com> wrote: > >> If x is 0, 'x - 1' is in the previous line, or worse outside the buffer > >> for the first line. > >> > >> If y is 0, 'x - image->comps[compno].w' is outside the buffer. > >> > > > > I'm slightly puzzled, as you say, these are for edge handling, edges > > in this case are from the image width to buffer width, and image > > height to buffer height, respectively > > So for x or y to be zero, we would need an image thats zero width, or > > zero height, so the edge starts at zero? > > > > How does that happen, and wouldn't it be much simpler to catch that > > case earlier in the chain and simply error out? A image with either > > zero width or zero height surely is not something you can encode > > either way. > > The avctx->width/avctx->height is not zero, but libopenjpeg_copy_unpacked8 > does: > width = avctx->width / image->comps[compno].dx; > height = avctx->height / image->comps[compno].dy; this looks wrong to me the code in mj2_create_image() looks better: cmptparm[i].dx = sub_dx[i]; cmptparm[i].dy = sub_dy[i]; cmptparm[i].w = (avctx->width + sub_dx[i] - 1) / sub_dx[i]; cmptparm[i].h = (avctx->height + sub_dy[i] - 1) / sub_dy[i]; i assume here that the buffers are large enough and that the encoder encodes the full rounded up size if it does not then it doesnt support odd sizes with subsampling > > So if e.g. avctx->height is 1 and image->comps[compno].dy is 2, height > becomes 0. I'm not sure if that's invalid. chroma planes being 0 sized is invalid for a normal image you can potentially get this with tiling that some tiles have no chroma samples but i think the code doesnt do anything odd like that of course i could also be missing somethig [...]
diff --git a/libavcodec/libopenjpegenc.c b/libavcodec/libopenjpegenc.c index 857ee1a..83c965d 100644 --- a/libavcodec/libopenjpegenc.c +++ b/libavcodec/libopenjpegenc.c @@ -415,13 +415,13 @@ static int libopenjpeg_copy_packed8(AVCodecContext *avctx, const AVFrame *frame, frame_index += numcomps; } for (; x < image->comps[compno].w; ++x) { - image_line[x] = image_line[x - 1]; + image_line[x] = x > 0 ? image_line[x - 1] : 0; } } for (; y < image->comps[compno].h; ++y) { image_line = image->comps[compno].data + y * image->comps[compno].w; for (x = 0; x < image->comps[compno].w; ++x) { - image_line[x] = image_line[x - image->comps[compno].w]; + image_line[x] = y > 0 ? image_line[x - (int)image->comps[compno].w] : 0; } } } @@ -455,13 +455,13 @@ static int libopenjpeg_copy_packed12(AVCodecContext *avctx, const AVFrame *frame frame_index += numcomps; } for (; x < image->comps[compno].w; ++x) { - image_line[x] = image_line[x - 1]; + image_line[x] = x > 0 ? image_line[x - 1] : 0; } } for (; y < image->comps[compno].h; ++y) { image_line = image->comps[compno].data + y * image->comps[compno].w; for (x = 0; x < image->comps[compno].w; ++x) { - image_line[x] = image_line[x - image->comps[compno].w]; + image_line[x] = y > 0 ? image_line[x - (int)image->comps[compno].w] : 0; } } } @@ -495,13 +495,13 @@ static int libopenjpeg_copy_packed16(AVCodecContext *avctx, const AVFrame *frame frame_index += numcomps; } for (; x < image->comps[compno].w; ++x) { - image_line[x] = image_line[x - 1]; + image_line[x] = x > 0 ? image_line[x - 1] : 0; } } for (; y < image->comps[compno].h; ++y) { image_line = image->comps[compno].data + y * image->comps[compno].w; for (x = 0; x < image->comps[compno].w; ++x) { - image_line[x] = image_line[x - image->comps[compno].w]; + image_line[x] = y > 0 ? image_line[x - (int)image->comps[compno].w] : 0; } } } @@ -536,13 +536,13 @@ static int libopenjpeg_copy_unpacked8(AVCodecContext *avctx, const AVFrame *fram for (x = 0; x < width; ++x) image_line[x] = frame->data[compno][frame_index++]; for (; x < image->comps[compno].w; ++x) { - image_line[x] = image_line[x - 1]; + image_line[x] = x > 0 ? image_line[x - 1] : 0; } } for (; y < image->comps[compno].h; ++y) { image_line = image->comps[compno].data + y * image->comps[compno].w; for (x = 0; x < image->comps[compno].w; ++x) { - image_line[x] = image_line[x - image->comps[compno].w]; + image_line[x] = y > 0 ? image_line[x - (int)image->comps[compno].w] : 0; } } } @@ -579,13 +579,13 @@ static int libopenjpeg_copy_unpacked16(AVCodecContext *avctx, const AVFrame *fra for (x = 0; x < width; ++x) image_line[x] = frame_ptr[frame_index++]; for (; x < image->comps[compno].w; ++x) { - image_line[x] = image_line[x - 1]; + image_line[x] = x > 0 ? image_line[x - 1] : 0; } } for (; y < image->comps[compno].h; ++y) { image_line = image->comps[compno].data + y * image->comps[compno].w; for (x = 0; x < image->comps[compno].w; ++x) { - image_line[x] = image_line[x - image->comps[compno].w]; + image_line[x] = y > 0 ? image_line[x - (int)image->comps[compno].w] : 0; } } }
If x is 0, 'x - 1' is in the previous line, or worse outside the buffer for the first line. If y is 0, 'x - image->comps[compno].w' is outside the buffer. Finally, image->comps[compno].w is unsigned (at least in openjpeg2), so the calculation could silently wrap around without the explicit cast to int. Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavcodec/libopenjpegenc.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)