On 7/8/2020 10:50 PM, Brian Kim wrote: > Patch attached. > > Main changes from v1 are switching to from int to size_t/ptrdiff_t > where relevant and removing av_image_fill_pointers_from_sizes() > > Some things to note: > - The av_image_fill_planes_sizes uses size_t/ptrdiff_t for buffer > sizes, but as mentioned during the v1 review, this leads to some > inconvenient conversions and range checks when using it with existing > functions. We could keep the return type as int to alleviate this, but > that seems like it would somewhat defeat the purpose of moving to > these types. > - SIZE_MAX is used in several places in libavutil, so I used > PTRDIFF_MAX, but I could not see any mention of these being allowed.
[...] > From a47b3f3b5c2ed4a1caf9f0a3429dd425ce708bb1 Mon Sep 17 00:00:00 2001 > From: Brian Kim <bk...@google.com> > Date: Tue, 7 Jul 2020 11:36:19 -0700 > Subject: [PATCH 1/3] libavutil/imgutils: add utility to get plane sizes > > This utility helps avoid undefined behavior when doing things like > checking how much memory we need to allocate for an image before we have > allocated a buffer. > > Signed-off-by: Brian Kim <bk...@google.com> > --- > doc/APIchanges | 3 ++ > libavutil/frame.c | 15 ++++++--- > libavutil/imgutils.c | 74 ++++++++++++++++++++++++++++++++------------ > libavutil/imgutils.h | 12 +++++++ > libavutil/version.h | 2 +- > 5 files changed, 82 insertions(+), 24 deletions(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 1d6cc36b8c..44defd9ca8 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -15,6 +15,9 @@ libavutil: 2017-10-21 > > API changes, most recent first: > > +2020-07-xx - xxxxxxxxxx - lavu 56.56.100 - imgutils.h > + Add av_image_fill_plane_sizes(). > + > 2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h > Add AV_PIX_FMT_X2RGB10. > > diff --git a/libavutil/frame.c b/libavutil/frame.c > index 9884eae054..b664dcfbe8 100644 > --- a/libavutil/frame.c > +++ b/libavutil/frame.c Changes to frame.c should be in a separate commit as well. > @@ -214,6 +214,8 @@ static int get_video_buffer(AVFrame *frame, int align) > const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format); > int ret, i, padded_height; > int plane_padding = FFMAX(16 + 16/*STRIDE_ALIGN*/, align); > + ptrdiff_t total_size, linesizes[4]; > + size_t size[4]; > > if (!desc) > return AVERROR(EINVAL); > @@ -238,12 +240,17 @@ static int get_video_buffer(AVFrame *frame, int align) > frame->linesize[i] = FFALIGN(frame->linesize[i], align); > } > > + for (i = 0; i < 4; i++) > + linesizes[i] = frame->linesize[i]; > + > padded_height = FFALIGN(frame->height, 32); > - if ((ret = av_image_fill_pointers(frame->data, frame->format, > padded_height, > - NULL, frame->linesize)) < 0) > - return ret; > + if ((total_size = av_image_fill_plane_sizes(size, frame->format, > padded_height, > + linesizes)) < 0) > + return total_size; > + if (total_size > INT_MAX - 4*plane_padding) > + return AVERROR(EINVAL); > > - frame->buf[0] = av_buffer_alloc(ret + 4*plane_padding); > + frame->buf[0] = av_buffer_alloc(total_size + 4*plane_padding); > if (!frame->buf[0]) { > ret = AVERROR(ENOMEM); > goto fail; > diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c > index 7f9c1b632c..082229cfaf 100644 > --- a/libavutil/imgutils.c > +++ b/libavutil/imgutils.c > @@ -108,26 +108,26 @@ int av_image_fill_linesizes(int linesizes[4], enum > AVPixelFormat pix_fmt, int wi > return 0; > } > > -int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int > height, > - uint8_t *ptr, const int linesizes[4]) > +ptrdiff_t av_image_fill_plane_sizes(size_t size[4], enum AVPixelFormat > pix_fmt, The sum of all sizes should be size_t if each size is a size_t. But if you do that you can't return error values, so i'm not sure what to suggest other than just stick to ints for both (ptrdiff_t linesizes should be ok). I'd like to hear Michael's opinion about it. For that matter, as it is right now i think this would be the first function that returns ptrdiff_t to signal AVERROR values. > + int height, const ptrdiff_t linesizes[4]) > { > - int i, total_size, size[4] = { 0 }, has_plane[4] = { 0 }; > + int i, has_plane[4] = { 0 }; > + ptrdiff_t total_size; > > const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pix_fmt); > - memset(data , 0, sizeof(data[0])*4); > + memset(size , 0, sizeof(size[0])*4); > > if (!desc || desc->flags & AV_PIX_FMT_FLAG_HWACCEL) > return AVERROR(EINVAL); > > - data[0] = ptr; > - if (linesizes[0] > (INT_MAX - 1024) / height) > + if (linesizes[0] > (PTRDIFF_MAX - 1024) / height) This check would need to ensure the calculation below fits in a size_t instead. Same for other similar calculations in the function. > return AVERROR(EINVAL); > size[0] = linesizes[0] * height; _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".