| Message ID | 20230516020624.71232-1-haihao.xiang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] lavu/frame: add av_frame_get_buffer2 | expand |
On Tue, May 16, 2023 at 4:07 AM Xiang, Haihao <haihao.xiang-at-intel.com@ffmpeg.org> wrote: > > From: Haihao Xiang <haihao.xiang@intel.com> > > Intel MediaSDK and oneVPL expect contiguous allocation for data[i], > however there are mandatory padding bytes between data[i] and data[i+1]. > when calling av_frame_get_buffer. So adding av_frame_get_buffer2 to > allow caller to specify the length of padding bytes. > get_video_buffer will also pad the height to a multiple of 32, won't that again cause issues? I don't think the API even necessarily guarantees that its going to be one contiguous buffer, it just happens to be the way its made right now. If a new API is introduced, maybe it should be very tailor designed to offer these guarantees, and named appropriate, not introducing any padding - padded height or extra padding, or otherwise. - Hendrik
> On May 16, 2023, at 15:52, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > On Tue, May 16, 2023 at 4:07 AM Xiang, Haihao > <haihao.xiang-at-intel.com@ffmpeg.org> wrote: >> >> From: Haihao Xiang <haihao.xiang@intel.com> >> >> Intel MediaSDK and oneVPL expect contiguous allocation for data[i], >> however there are mandatory padding bytes between data[i] and data[i+1]. >> when calling av_frame_get_buffer. So adding av_frame_get_buffer2 to >> allow caller to specify the length of padding bytes. >> > > get_video_buffer will also pad the height to a multiple of 32, won't > that again cause issues? > I don't think the API even necessarily guarantees that its going to be > one contiguous buffer, it just happens to be the way its made right > now. > > If a new API is introduced, maybe it should be very tailor designed to > offer these guarantees, and named appropriate, not introducing any > padding - padded height or extra padding, or otherwise. If there is more use cases, the new API should be more general. Current use case isn’t a strong reason for the new API. It looks like the internal implementation of hwcontext_qsv and qsvenc use AVFrame as temporary variables for convenience. They can be replaced by internal defined struct or POD, and copy data by av_image_copy. Although it’s more code to change. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Di, 2023-05-16 at 16:22 +0800, zhilizhao(赵志立) wrote: > > > On May 16, 2023, at 15:52, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > > > On Tue, May 16, 2023 at 4:07 AM Xiang, Haihao > > <haihao.xiang-at-intel.com@ffmpeg.org> wrote: > > > > > > From: Haihao Xiang <haihao.xiang@intel.com> > > > > > > Intel MediaSDK and oneVPL expect contiguous allocation for data[i], > > > however there are mandatory padding bytes between data[i] and data[i+1]. > > > when calling av_frame_get_buffer. So adding av_frame_get_buffer2 to > > > allow caller to specify the length of padding bytes. > > > > > > > get_video_buffer will also pad the height to a multiple of 32, won't > > that again cause issues? > > I don't think the API even necessarily guarantees that its going to be > > one contiguous buffer, it just happens to be the way its made right > > now. > > > > If a new API is introduced, maybe it should be very tailor designed to > > offer these guarantees, and named appropriate, not introducing any > > padding - padded height or extra padding, or otherwise. > > If there is more use cases, the new API should be more general. > Current use case isn’t a strong reason for the new API. > > It looks like the internal implementation of hwcontext_qsv and > qsvenc use AVFrame as temporary variables for convenience. They > can be replaced by internal defined struct or POD, and copy data > by av_image_copy. Although it’s more code to change. > Thanks all for the comments, I'll fix this in qsv with a new patchset. BRs Haihao
diff --git a/doc/APIchanges b/doc/APIchanges index 20ab4709e7..6a2c3b6270 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09 API changes, most recent first: +2023-05-09 - xxxxxxxxxx - lavu 58.8.100 - frame.h + Add av_frame_get_buffer2 + 2023-05-xx - xxxxxxxxxx - lavc 60.11.100 - codec_par.h Add AVCodecParameters.framerate. diff --git a/libavutil/frame.c b/libavutil/frame.c index 97d40208c8..5c9e740c6b 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -106,11 +106,10 @@ void av_frame_free(AVFrame **frame) av_freep(frame); } -static int get_video_buffer(AVFrame *frame, int align) +static int get_video_buffer(AVFrame *frame, int align, int plane_padding) { const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format); int ret, padded_height, total_size; - int plane_padding = FFMAX(16 + 16/*STRIDE_ALIGN*/, align); ptrdiff_t linesizes[4]; size_t sizes[4]; @@ -240,14 +239,14 @@ FF_ENABLE_DEPRECATION_WARNINGS } -int av_frame_get_buffer(AVFrame *frame, int align) +int av_frame_get_buffer2(AVFrame *frame, int align, int plane_padding) { if (frame->format < 0) return AVERROR(EINVAL); FF_DISABLE_DEPRECATION_WARNINGS if (frame->width > 0 && frame->height > 0) - return get_video_buffer(frame, align); + return get_video_buffer(frame, align, plane_padding); else if (frame->nb_samples > 0 && (av_channel_layout_check(&frame->ch_layout) #if FF_API_OLD_CHANNEL_LAYOUT @@ -260,6 +259,11 @@ FF_ENABLE_DEPRECATION_WARNINGS return AVERROR(EINVAL); } +int av_frame_get_buffer(AVFrame *frame, int align) +{ + return av_frame_get_buffer2(frame, align, FFMAX(16 + 16/*STRIDE_ALIGN*/, align)); +} + static int frame_copy_props(AVFrame *dst, const AVFrame *src, int force_copy) { int ret; diff --git a/libavutil/frame.h b/libavutil/frame.h index f2b56beebb..197bcfeef2 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -867,6 +867,37 @@ void av_frame_move_ref(AVFrame *dst, AVFrame *src); */ int av_frame_get_buffer(AVFrame *frame, int align); +/** + * Allocate new buffer(s) for audio or video data. + * + * The following fields must be set on frame before calling this function: + * - format (pixel format for video, sample format for audio) + * - width and height for video + * - nb_samples and ch_layout for audio + * + * This function will fill AVFrame.data and AVFrame.buf arrays and, if + * necessary, allocate and fill AVFrame.extended_data and AVFrame.extended_buf. + * For planar formats, one buffer will be allocated for each plane. + * + * @warning: if frame already has been allocated, calling this function will + * leak memory. In addition, undefined behavior can occur in certain + * cases. + * + * @param frame frame in which to store the new buffers. + * @param align Required buffer size alignment. If equal to 0, alignment will be + * chosen automatically for the current CPU. It is highly + * recommended to pass 0 here unless you know what you are doing. + * @param plane_padding The length of padding bytes between two video planes. It is + * ignored for audio data. + * + * @return 0 on success, a negative AVERROR on error. + * + * @note It is recommended that you use av_frame_get_buffer instead if you do not + * care about the length of padding bytes. + * @see av_frame_get_buffer() + */ +int av_frame_get_buffer2(AVFrame *frame, int align, int plane_padding); + /** * Check if the frame data is writable. * diff --git a/libavutil/version.h b/libavutil/version.h index 341bcbf188..a4177aa137 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 58 -#define LIBAVUTIL_VERSION_MINOR 7 +#define LIBAVUTIL_VERSION_MINOR 8 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \