| Message ID | 8eb9290f3c9cdae75b38c45a5d49ee624951a849.camel@amazon.it |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] Optimization: support for libx264's mb_info | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
On date Friday 2023-05-19 10:19:03 +0000, Carotti, Elias wrote: > > Hi again, > I am sending this patch again (I had missed a check for NULL), could > somebody please have a look at it? > > It is mainly an optimization when the encoder knows in advance that > only portions of the whole frame changed and which areas actually did. > > The patch allows a user to pass down information to libx264 information > about which parts of a frame changed (with respect to the preceding > one) to be exploited as a hint for P_SKIP-ing macroblocks which didn't > change. > This information is encoded in the mb_info field of the x264_param_t > much like the quant_offsets which are already used for the > AV_FRAME_DATA_REGIONS_OF_INTEREST side information. Please send a git format-patch (complete with commit log information) to simplify integration. [...] > diff --git a/libavutil/mb_info.h b/libavutil/mb_info.h > new file mode 100644 > index 0000000000..918cf167aa > --- /dev/null > +++ b/libavutil/mb_info.h > @@ -0,0 +1,46 @@ [...] > +#ifndef AVUTIL_MB_INFO_H > +#define AVUTIL_MB_INFO_H > + > +#include <stddef.h> > +#include <stdint.h> > +#include "libavutil/avassert.h" > +#include "libavutil/frame.h" > + > +typedef struct _AVMBInfoRect { nit: strip the _ before the struct name > + uint32_t x, y; > + uint32_t width, height; > +} AVMBInfoRect; > + > +/** > + * Allocate memory for a vector of AVMBInfoRect in the given AVFrame > + * {@code frame} as AVFrameSideData of type AV_FRAME_DATA_MB_INFO. > + * The side data contains a list of rectangles for the portions of the frame > + * which changed from the last encoded one. The rest will be hinted to be > + * P_SKIP-ped. Portions of the rects which are not on macroblock boundaries > + * are not handled as P_SKIPS. > + */ > +AVMBInfoRect *av_mb_info_create_side_data(AVFrame *frame, > + AVMBInfoRect *rects, > + size_t num_rects); Probably this can be generalized with a flag defining the hinting type (you might want either to specify the constant or non-constant rects), in fact this data is pretty macro-block agnostic. What about AVVideoHintInfo containing a hint (CONSTANT, VARIANT) and a list of rects?
Hi Stefano, thanks for checking the patch. Please find it attached in the required format. Best, Elias On Mon, 2023-05-22 at 01:17 +0200, Stefano Sabatini wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > On date Friday 2023-05-19 10:19:03 +0000, Carotti, Elias wrote: > > > > Hi again, > > I am sending this patch again (I had missed a check for NULL), > > could > > somebody please have a look at it? > > > > It is mainly an optimization when the encoder knows in advance that > > only portions of the whole frame changed and which areas actually > > did. > > > > The patch allows a user to pass down information to libx264 > > information > > about which parts of a frame changed (with respect to the preceding > > one) to be exploited as a hint for P_SKIP-ing macroblocks which > > didn't > > change. > > This information is encoded in the mb_info field of the > > x264_param_t > > much like the quant_offsets which are already used for the > > AV_FRAME_DATA_REGIONS_OF_INTEREST side information. > > Please send a git format-patch (complete with commit log information) > to simplify integration. > > [...] > > diff --git a/libavutil/mb_info.h b/libavutil/mb_info.h > > new file mode 100644 > > index 0000000000..918cf167aa > > --- /dev/null > > +++ b/libavutil/mb_info.h > > @@ -0,0 +1,46 @@ > [...] > > +#ifndef AVUTIL_MB_INFO_H > > +#define AVUTIL_MB_INFO_H > > + > > +#include <stddef.h> > > +#include <stdint.h> > > +#include "libavutil/avassert.h" > > +#include "libavutil/frame.h" > > + > > > +typedef struct _AVMBInfoRect { > > nit: strip the _ before the struct name > > > + uint32_t x, y; > > + uint32_t width, height; > > +} AVMBInfoRect; > > + > > +/** > > + * Allocate memory for a vector of AVMBInfoRect in the given > > AVFrame > > + * {@code frame} as AVFrameSideData of type AV_FRAME_DATA_MB_INFO. > > > + * The side data contains a list of rectangles for the portions of > > the frame > > + * which changed from the last encoded one. The rest will be > > hinted to be > > + * P_SKIP-ped. Portions of the rects which are not on macroblock > > boundaries > > + * are not handled as P_SKIPS. > > + */ > > > +AVMBInfoRect *av_mb_info_create_side_data(AVFrame *frame, > > + AVMBInfoRect *rects, > > + size_t num_rects); > > Probably this can be generalized with a flag defining the hinting > type > (you might want either to specify the constant or non-constant > rects), > in fact this data is pretty macro-block agnostic. > > What about AVVideoHintInfo containing a hint (CONSTANT, VARIANT) and > a > list of rects? > _______________________________________________ > 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". NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
Hi Stefano, hi all, please find the updated patch according to the suggestions. Now it is possible to specify whether the rectangles refer to the part we want to hint as P_SKIP or to the rest portion of the frame. Best, Elias On Mon, 2023-05-22 at 09:19 +0000, Carotti, Elias wrote: > Hi Stefano, > thanks for checking the patch. > Please find it attached in the required format. > Best, > Elias > > > On Mon, 2023-05-22 at 01:17 +0200, Stefano Sabatini wrote: > > CAUTION: This email originated from outside of the organization. Do > > not click links or open attachments unless you can confirm the > > sender > > and know the content is safe. > > > > > > > > On date Friday 2023-05-19 10:19:03 +0000, Carotti, Elias wrote: > > > > > > Hi again, > > > I am sending this patch again (I had missed a check for NULL), > > > could > > > somebody please have a look at it? > > > > > > It is mainly an optimization when the encoder knows in advance > > > that > > > only portions of the whole frame changed and which areas actually > > > did. > > > > > > The patch allows a user to pass down information to libx264 > > > information > > > about which parts of a frame changed (with respect to the > > > preceding > > > one) to be exploited as a hint for P_SKIP-ing macroblocks which > > > didn't > > > change. > > > This information is encoded in the mb_info field of the > > > x264_param_t > > > much like the quant_offsets which are already used for the > > > AV_FRAME_DATA_REGIONS_OF_INTEREST side information. > > > > Please send a git format-patch (complete with commit log > > information) > > to simplify integration. > > > > [...] > > > diff --git a/libavutil/mb_info.h b/libavutil/mb_info.h > > > new file mode 100644 > > > index 0000000000..918cf167aa > > > --- /dev/null > > > +++ b/libavutil/mb_info.h > > > @@ -0,0 +1,46 @@ > > [...] > > > +#ifndef AVUTIL_MB_INFO_H > > > +#define AVUTIL_MB_INFO_H > > > + > > > +#include <stddef.h> > > > +#include <stdint.h> > > > +#include "libavutil/avassert.h" > > > +#include "libavutil/frame.h" > > > + > > > > > +typedef struct _AVMBInfoRect { > > > > nit: strip the _ before the struct name > > > > > + uint32_t x, y; > > > + uint32_t width, height; > > > +} AVMBInfoRect; > > > + > > > +/** > > > + * Allocate memory for a vector of AVMBInfoRect in the given > > > AVFrame > > > + * {@code frame} as AVFrameSideData of type > > > AV_FRAME_DATA_MB_INFO. > > > > > + * The side data contains a list of rectangles for the portions > > > of > > > the frame > > > + * which changed from the last encoded one. The rest will be > > > hinted to be > > > + * P_SKIP-ped. Portions of the rects which are not on > > > macroblock > > > boundaries > > > + * are not handled as P_SKIPS. > > > + */ > > > > > +AVMBInfoRect *av_mb_info_create_side_data(AVFrame *frame, > > > + AVMBInfoRect *rects, > > > + size_t num_rects); > > > > Probably this can be generalized with a flag defining the hinting > > type > > (you might want either to specify the constant or non-constant > > rects), > > in fact this data is pretty macro-block agnostic. > > > > What about AVVideoHintInfo containing a hint (CONSTANT, VARIANT) > > and > > a > > list of rects? > > _______________________________________________ > > 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". > > > > > NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro > delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale > Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa > con Socio Unico > > > _______________________________________________ > 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". NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On date Monday 2023-05-29 17:56:55 +0000, Carotti, Elias wrote: [...] > From 7cb97ee977197e310a932b2d7a53bf5c6e99990e Mon Sep 17 00:00:00 2001 > From: Elias Carotti <eliascrt@amazon.it> > Date: Wed, 19 Apr 2023 11:49:39 +0200 > Subject: [PATCH] Add support for libx264's MB_INFO > > libx264's x264_image_properties_t, which is passed to the encoding function, > contains a field to pass down information on the portions of the frame which > changed with respect to the previous one (used for prediction) to mark > unchanged macroblocks P_SKIP. > --- > libavcodec/libx264.c | 81 +++++++++++++++++++++++++++++++++ > libavutil/Makefile | 4 ++ > libavutil/frame.h | 10 +++++ > libavutil/video_hint_info.c | 89 +++++++++++++++++++++++++++++++++++++ > libavutil/video_hint_info.h | 87 ++++++++++++++++++++++++++++++++++++ > 5 files changed, 271 insertions(+) > create mode 100644 libavutil/video_hint_info.c > create mode 100644 libavutil/video_hint_info.h > > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c > index 5736f1efa7..32fa80d0a1 100644 > --- a/libavcodec/libx264.c > +++ b/libavcodec/libx264.c > @@ -30,6 +30,7 @@ > #include "libavutil/stereo3d.h" > #include "libavutil/time.h" > #include "libavutil/intreadwrite.h" > +#include "libavutil/video_hint_info.h" > #include "avcodec.h" > #include "codec_internal.h" > #include "encode.h" > @@ -48,6 +49,9 @@ > // from x264.h, for quant_offsets, Macroblocks are 16x16 > // blocks of pixels (with respect to the luma plane) > #define MB_SIZE 16 > +#define MB_LSIZE 4 > +#define MB_FLOOR(x) ((x) >> (MB_LSIZE)) > +#define MB_CEIL(x) MB_FLOOR((x) + (MB_SIZE - 1)) > > typedef struct X264Opaque { > #if FF_API_REORDERED_OPAQUE > @@ -123,6 +127,8 @@ typedef struct X264Context { > * encounter a frame with ROI side data. > */ > int roi_warned; > + > + int mb_info; > } X264Context; > > static void X264_log(void *p, int level, const char *fmt, va_list args) > @@ -295,6 +301,7 @@ static void free_picture(x264_picture_t *pic) > av_free(pic->extra_sei.payloads[i].payload); > av_freep(&pic->extra_sei.payloads); > av_freep(&pic->prop.quant_offsets); > + av_freep(&pic->prop.mb_info); > pic->extra_sei.num_payloads = 0; > } > > @@ -320,6 +327,64 @@ static enum AVPixelFormat csp_to_pixfmt(int csp) > return AV_PIX_FMT_NONE; > } > > +static int setup_mb_info(AVCodecContext *ctx, x264_picture_t *pic, > + const AVFrame *frame, > + const AVVideoHint *info) > +{ > + int mb_width = (frame->width + MB_SIZE - 1) / MB_SIZE; > + int mb_height = (frame->height + MB_SIZE - 1) / MB_SIZE; > + > + const AVVideoRect *mbinfo_rects; > + int nb_rects; > + uint8_t *mbinfo; > + > + mbinfo_rects = (const AVVideoRect *)av_video_hint_rects(info); > + nb_rects = info->nb_rects; > + > + mbinfo = av_calloc(mb_width * mb_height, sizeof(*mbinfo)); > + if (!mbinfo) > + return AVERROR(ENOMEM); > + > + if (info->type == AV_VIDEO_HINT_CHANGED) { > + /* Sets the default as constant, i.e. P_SKIP-able, then selectively resets the flag */ > + memset(mbinfo, X264_MBINFO_CONSTANT, sizeof(*mbinfo) * mb_width * mb_height); > + > + for (int i = 0; i < nb_rects; i++) { > + int min_y = MB_FLOOR(mbinfo_rects->y); > + int max_y = MB_CEIL(mbinfo_rects->y + mbinfo_rects->height); > + int min_x = MB_FLOOR(mbinfo_rects->x); > + int max_x = MB_CEIL(mbinfo_rects->x + mbinfo_rects->width); > + > + for (int mb_y = min_y; mb_y < max_y; ++mb_y) { > + memset(mbinfo + mb_y * mb_width + min_x, 0, max_x - min_x); > + } > + > + mbinfo_rects++; > + } > + } else { > + /* Sets the default as changed, i.e. *not* P_SKIP-able, then selectively sets the flag */ > + memset(mbinfo, 0, sizeof(*mbinfo) * mb_width * mb_height); > + > + for (int i = 0; i < nb_rects; i++) { > + int min_y = MB_CEIL(mbinfo_rects->y); > + int max_y = MB_FLOOR(mbinfo_rects->y + mbinfo_rects->height); > + int min_x = MB_CEIL(mbinfo_rects->x); > + int max_x = MB_FLOOR(mbinfo_rects->x + mbinfo_rects->width); > + > + for (int mb_y = min_y; mb_y < max_y; ++mb_y) { > + memset(mbinfo + mb_y * mb_width + min_x, X264_MBINFO_CONSTANT, max_x - min_x); > + } > + > + mbinfo_rects++; you can merge the two loops by adding an inline function to compute the mins and maxs, and then setting: mbinfo_filler = AV_VIDEO_HINT_CHANGED ? X264_MBINFO_CONSTANT : 0; mbinfo_marker = AV_VIDEO_HINT_CHANGED ? 0 : X264_MBINFO_CONSTANT; compute_coords_fn = AV_VIDEO_HINT_CHANGED ? compute_changed_coords : compute_constant_coords; memset(mbinfo, mbinfo_filler, sizeof(*mbinfo) * mb_width * mb_height); for (int i = 0; i < nb_rects; i++) { compute_coords_fn... for (int mb_y = min_y; mb_y < max_y; ++mb_y) { memset(mbinfo + mb_y * mb_width + min_x, mbinfo_marker, max_x - min_x); } mbinfo_rects++; } > + } > + } > + > + pic->prop.mb_info = mbinfo; > + pic->prop.mb_info_free = av_free; > + > + return 0; > +} > + > static int setup_roi(AVCodecContext *ctx, x264_picture_t *pic, int bit_depth, > const AVFrame *frame, const uint8_t *data, size_t size) > { > @@ -404,6 +469,7 @@ static int setup_frame(AVCodecContext *ctx, const AVFrame *frame, > int64_t wallclock = 0; > int bit_depth, ret; > AVFrameSideData *sd; > + AVFrameSideData *mbinfo_sd; > > *ppic = NULL; > if (!frame) > @@ -499,6 +565,17 @@ FF_ENABLE_DEPRECATION_WARNINGS > goto fail; > } > > + mbinfo_sd = av_frame_get_side_data(frame, AV_FRAME_DATA_VIDEO_HINT); > + if (mbinfo_sd) { > + int ret = setup_mb_info(ctx, pic, frame, (const AVVideoHint *)mbinfo_sd->data); > + if (ret < 0) { > + /* No need to fail here, this is not fatal. We just proceed with no > + * mb_info and log a message */ > + > + av_log(ctx, AV_LOG_WARNING, "mb_info setup failure\n"); > + } > + } > + > if (x4->udu_sei) { > for (int j = 0; j < frame->nb_side_data; j++) { > AVFrameSideData *side_data = frame->side_data[j]; > @@ -1102,6 +1179,9 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > } > > + x4->params.analyse.b_mb_info = x4->mb_info; > + x4->params.analyse.b_fast_pskip = 1; > + > // update AVCodecContext with x264 parameters > avctx->has_b_frames = x4->params.i_bframe ? > x4->params.i_bframe_pyramid ? 2 : 1 : 0; > @@ -1311,6 +1391,7 @@ static const AVOption options[] = { > { "noise_reduction", "Noise reduction", OFFSET(noise_reduction), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE }, > { "udu_sei", "Use user data unregistered SEI if available", OFFSET(udu_sei), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, > { "x264-params", "Override the x264 configuration using a :-separated list of key=value parameters", OFFSET(x264_params), AV_OPT_TYPE_DICT, { 0 }, 0, 0, VE }, > + { "mb_info", "Set mb_info data through AVSideData, only useful when used from the API", OFFSET(mb_info), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, > { NULL }, > }; > > diff --git a/libavutil/Makefile b/libavutil/Makefile > index dc9012f9a8..bb5ecc3235 100644 > --- a/libavutil/Makefile > +++ b/libavutil/Makefile > @@ -91,6 +91,7 @@ HEADERS = adler32.h \ > tea.h \ > tx.h \ > film_grain_params.h \ > + video_hint_info.h \ > > ARCH_HEADERS = bswap.h \ > intmath.h \ > @@ -196,6 +197,7 @@ OBJS-$(CONFIG_VAAPI) += hwcontext_vaapi.o > OBJS-$(CONFIG_VIDEOTOOLBOX) += hwcontext_videotoolbox.o > OBJS-$(CONFIG_VDPAU) += hwcontext_vdpau.o > OBJS-$(CONFIG_VULKAN) += hwcontext_vulkan.o > +OBJS-$(CONFIG_LIBX264) += video_hint_info.o order > > OBJS-$(!CONFIG_VULKAN) += hwcontext_stub.o > > @@ -219,6 +221,8 @@ SKIPHEADERS-$(CONFIG_VULKAN) += hwcontext_vulkan.h vulkan.h \ > vulkan_functions.h \ > vulkan_loader.h > > +SKIPHEADERS-$(CONFIG_LIBX264) += video_hint_info.h > + > TESTPROGS = adler32 \ > aes \ > aes_ctr \ > diff --git a/libavutil/frame.h b/libavutil/frame.h > index a491315f25..0e765e5499 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -214,6 +214,16 @@ enum AVFrameSideDataType { > * Ambient viewing environment metadata, as defined by H.274. > */ > AV_FRAME_DATA_AMBIENT_VIEWING_ENVIRONMENT, > + > + /** > + * Provide macro block encoder-specific hinting information for the encoder > + * processing. It can be used to pass information about which macroblock > + * can be skipped because it hasn't changed from the corresponding one in > + * the previous frame. This is useful for applications which know in > + * advance this information to speed up real-time encoding. Currently only > + * used by libx264. > + */ > + AV_FRAME_DATA_VIDEO_HINT, > }; > > enum AVActiveFormatDescription { > diff --git a/libavutil/video_hint_info.c b/libavutil/video_hint_info.c > new file mode 100644 > index 0000000000..0a763962da > --- /dev/null > +++ b/libavutil/video_hint_info.c > @@ -0,0 +1,89 @@ > +/* > + * Copyright 2023 Elias Carotti <eliascrt at amazon dot it> > + * > + * This file is part of FFmpeg. > + * > + * FFmpeg is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * FFmpeg is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + */ > + > +#include <string.h> > + > +#include "avstring.h" > +#include "frame.h" > +#include "macros.h" > +#include "mem.h" > +#include "video_hint_info.h" > + > +AVVideoHint *av_video_hint_alloc(AVVideoRect *rects, > + size_t nb_rects, > + VideoHintType type, > + size_t* out_size) align to ( > +{ > + struct TestStruct { > + AVVideoHint p; > + AVVideoRect b; > + }; nit: weird align (also use more meaningful names - e.g. hint and rect) > + const size_t blocks_offset = offsetof(struct TestStruct, b); > + size_t size = blocks_offset; > + AVVideoHint *par; AVVideoHint *hint? > + > + *out_size = 0; > + if (nb_rects > (SIZE_MAX - size) / sizeof(AVVideoRect)) why SIZE_MAX - size? > + return NULL; > + size += sizeof(AVVideoRect) * nb_rects; > + > + par = av_mallocz(size); > + if (!par) > + return NULL; > + > + par->type = type; > + par->nb_rects = nb_rects; > + par->blocks_offset = blocks_offset; > + > + /* Just copies the rects over the newly allocated buffer */ > + memcpy((uint8_t *)par + blocks_offset, rects, sizeof(AVVideoRect) * nb_rects); > + > + *out_size = size; > + > + return par; > +} > + > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > + AVVideoRect *rects, > + size_t num_rects, > + VideoHintType type) > +{ > + AVVideoHint *par; use a more meaningful name (I think "par" is a leftover of an older API) > + AVBufferRef *buf; > + size_t size = 0; > + > + par = av_video_hint_alloc(rects, num_rects, type, &size); > + if (!par) > + return NULL; > + > + buf = av_buffer_create((uint8_t *)par, size, NULL, NULL, 0); > + if (!buf) { > + av_freep(&par); > + return NULL; > + } > + > + if (!av_frame_new_side_data_from_buf(frame, AV_FRAME_DATA_VIDEO_HINT, buf)) { > + av_buffer_unref(&buf); > + return NULL; > + } > + > + return par; > +} > + > diff --git a/libavutil/video_hint_info.h b/libavutil/video_hint_info.h > new file mode 100644 > index 0000000000..3b04e0c40e > --- /dev/null > +++ b/libavutil/video_hint_info.h > @@ -0,0 +1,87 @@ > +/** > + * Copyright 2023 Elias Carotti <eliascrt at amazon dot it> > + * > + * This file is part of FFmpeg. > + * > + * FFmpeg is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * FFmpeg is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + */ > + > +#ifndef AVUTIL_VIDEO_HINT_INFO_H > +#define AVUTIL_VIDEO_HINT_INFO_H > + > +#include <stddef.h> > +#include <stdint.h> > +#include "libavutil/avassert.h" > +#include "libavutil/frame.h" > + > +typedef struct AVVideoRect { > + uint32_t x, y; > + uint32_t width, height; > +} AVVideoRect; > + > +typedef enum VideoHintType { > + /* blocks delimit the constant areas (unchanged), default is changed */ > + AV_VIDEO_HINT_CONSTANT, > + > + /* blocks delimit the constant areas (changed), default is not changed */ > + AV_VIDEO_HINT_CHANGED, > +} VideoHintType; missing AV prefix > + > +typedef struct AVVideoHint { > + /** > + * Number of blocks in the array. > + * > + * May be 0, in which case no per-block information is present. In this case > + * the values of blocks_offset / block_size are unspecified and should not > + * be accessed. > + */ > + int nb_rects; > + > + /** > + * Offset in bytes from the beginning of this structure at which the array > + * of blocks starts. > + */ > + size_t blocks_offset; > + > + VideoHintType type; > +} AVVideoHint; > + > +static av_always_inline AVVideoRect* > +av_video_hint_rects(const AVVideoHint *par) > +{ > + return (AVVideoRect *)((uint8_t *)par + par->blocks_offset); > +} > + > +/** > + * Allocate memory for a vector of AVVideoRect in the given AVFrame > + * {@code frame} as AVFrameSideData of type AV_FRAME_DATA_VIDEO_HINT_INFO. > + * The side data contains a list of rectangles for the portions of the frame > + * which changed from the last encoded one (and the remainder are assumed to be > + * changed), or, alternately (depending on the type parameter) the unchanged > + * ones (and the remanining ones are those which changed). > + * Macroblocks will thus be hinted either to be P_SKIP-ped or go through the > + * regular encoding procedure. > + */ > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > + AVVideoRect *rects, > + size_t num_rects, > + VideoHintType type); > + > +AVVideoHint *av_video_hint_alloc(AVVideoRect *rects, > + size_t nb_rects, > + VideoHintType type, > + size_t *out_size); > + > +#endif /* AVUTIL__VIDEO_HINT_INFO_H */ AVUTIL_VIDEO... > -- > 2.34.1 > > _______________________________________________ > 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".
Hi, please find attached the patch which I updated according to your suggestions. Best, Elias On Sun, 2023-06-04 at 17:29 +0200, Stefano Sabatini wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > On date Monday 2023-05-29 17:56:55 +0000, Carotti, Elias wrote: > [...] > > From 7cb97ee977197e310a932b2d7a53bf5c6e99990e Mon Sep 17 00:00:00 > > 2001 > > From: Elias Carotti <eliascrt@amazon.it> > > Date: Wed, 19 Apr 2023 11:49:39 +0200 > > Subject: [PATCH] Add support for libx264's MB_INFO > > > > libx264's x264_image_properties_t, which is passed to the encoding > > function, > > contains a field to pass down information on the portions of the > > frame which > > changed with respect to the previous one (used for prediction) to > > mark > > unchanged macroblocks P_SKIP. > > --- > > libavcodec/libx264.c | 81 +++++++++++++++++++++++++++++++++ > > libavutil/Makefile | 4 ++ > > libavutil/frame.h | 10 +++++ > > libavutil/video_hint_info.c | 89 > > +++++++++++++++++++++++++++++++++++++ > > libavutil/video_hint_info.h | 87 > > ++++++++++++++++++++++++++++++++++++ > > 5 files changed, 271 insertions(+) > > create mode 100644 libavutil/video_hint_info.c > > create mode 100644 libavutil/video_hint_info.h > > > > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c > > index 5736f1efa7..32fa80d0a1 100644 > > --- a/libavcodec/libx264.c > > +++ b/libavcodec/libx264.c > > @@ -30,6 +30,7 @@ > > #include "libavutil/stereo3d.h" > > #include "libavutil/time.h" > > #include "libavutil/intreadwrite.h" > > +#include "libavutil/video_hint_info.h" > > #include "avcodec.h" > > #include "codec_internal.h" > > #include "encode.h" > > @@ -48,6 +49,9 @@ > > // from x264.h, for quant_offsets, Macroblocks are 16x16 > > // blocks of pixels (with respect to the luma plane) > > #define MB_SIZE 16 > > +#define MB_LSIZE 4 > > +#define MB_FLOOR(x) ((x) >> (MB_LSIZE)) > > +#define MB_CEIL(x) MB_FLOOR((x) + (MB_SIZE - 1)) > > > > typedef struct X264Opaque { > > #if FF_API_REORDERED_OPAQUE > > @@ -123,6 +127,8 @@ typedef struct X264Context { > > * encounter a frame with ROI side data. > > */ > > int roi_warned; > > + > > + int mb_info; > > } X264Context; > > > > static void X264_log(void *p, int level, const char *fmt, va_list > > args) > > @@ -295,6 +301,7 @@ static void free_picture(x264_picture_t *pic) > > av_free(pic->extra_sei.payloads[i].payload); > > av_freep(&pic->extra_sei.payloads); > > av_freep(&pic->prop.quant_offsets); > > + av_freep(&pic->prop.mb_info); > > pic->extra_sei.num_payloads = 0; > > } > > > > @@ -320,6 +327,64 @@ static enum AVPixelFormat csp_to_pixfmt(int > > csp) > > return AV_PIX_FMT_NONE; > > } > > > > +static int setup_mb_info(AVCodecContext *ctx, x264_picture_t *pic, > > + const AVFrame *frame, > > + const AVVideoHint *info) > > +{ > > > + int mb_width = (frame->width + MB_SIZE - 1) / MB_SIZE; > > + int mb_height = (frame->height + MB_SIZE - 1) / MB_SIZE; > > > > + > > + const AVVideoRect *mbinfo_rects; > > + int nb_rects; > > + uint8_t *mbinfo; > > + > > + mbinfo_rects = (const AVVideoRect *)av_video_hint_rects(info); > > + nb_rects = info->nb_rects; > > + > > + mbinfo = av_calloc(mb_width * mb_height, sizeof(*mbinfo)); > > + if (!mbinfo) > > + return AVERROR(ENOMEM); > > + > > > + if (info->type == AV_VIDEO_HINT_CHANGED) { > > + /* Sets the default as constant, i.e. P_SKIP-able, then > > selectively resets the flag */ > > + memset(mbinfo, X264_MBINFO_CONSTANT, sizeof(*mbinfo) * > > mb_width * mb_height); > > + > > + for (int i = 0; i < nb_rects; i++) { > > + int min_y = MB_FLOOR(mbinfo_rects->y); > > + int max_y = MB_CEIL(mbinfo_rects->y + mbinfo_rects- > > >height); > > + int min_x = MB_FLOOR(mbinfo_rects->x); > > + int max_x = MB_CEIL(mbinfo_rects->x + mbinfo_rects- > > >width); > > + > > + for (int mb_y = min_y; mb_y < max_y; ++mb_y) { > > + memset(mbinfo + mb_y * mb_width + min_x, 0, max_x > > - min_x); > > + } > > + > > + mbinfo_rects++; > > + } > > + } else { > > + /* Sets the default as changed, i.e. *not* P_SKIP-able, > > then selectively sets the flag */ > > + memset(mbinfo, 0, sizeof(*mbinfo) * mb_width * mb_height); > > + > > + for (int i = 0; i < nb_rects; i++) { > > + int min_y = MB_CEIL(mbinfo_rects->y); > > + int max_y = MB_FLOOR(mbinfo_rects->y + mbinfo_rects- > > >height); > > + int min_x = MB_CEIL(mbinfo_rects->x); > > + int max_x = MB_FLOOR(mbinfo_rects->x + mbinfo_rects- > > >width); > > + > > + for (int mb_y = min_y; mb_y < max_y; ++mb_y) { > > + memset(mbinfo + mb_y * mb_width + min_x, > > X264_MBINFO_CONSTANT, max_x - min_x); > > + } > > + > > + mbinfo_rects++; > > you can merge the two loops by adding an inline function to compute > the mins and maxs, and then setting: > > mbinfo_filler = AV_VIDEO_HINT_CHANGED ? X264_MBINFO_CONSTANT : 0; > mbinfo_marker = AV_VIDEO_HINT_CHANGED ? 0 : X264_MBINFO_CONSTANT; > compute_coords_fn = AV_VIDEO_HINT_CHANGED ? compute_changed_coords : > compute_constant_coords; > > memset(mbinfo, mbinfo_filler, sizeof(*mbinfo) * mb_width * > mb_height); > for (int i = 0; i < nb_rects; i++) { > compute_coords_fn... > for (int mb_y = min_y; mb_y < max_y; ++mb_y) { > memset(mbinfo + mb_y * mb_width + min_x, mbinfo_marker, max_x > - min_x); > } > > mbinfo_rects++; > } > > > + } > > + } > > + > > + pic->prop.mb_info = mbinfo; > > + pic->prop.mb_info_free = av_free; > > + > > + return 0; > > +} > > + > > static int setup_roi(AVCodecContext *ctx, x264_picture_t *pic, int > > bit_depth, > > const AVFrame *frame, const uint8_t *data, > > size_t size) > > { > > @@ -404,6 +469,7 @@ static int setup_frame(AVCodecContext *ctx, > > const AVFrame *frame, > > int64_t wallclock = 0; > > int bit_depth, ret; > > AVFrameSideData *sd; > > + AVFrameSideData *mbinfo_sd; > > > > *ppic = NULL; > > if (!frame) > > @@ -499,6 +565,17 @@ FF_ENABLE_DEPRECATION_WARNINGS > > goto fail; > > } > > > > + mbinfo_sd = av_frame_get_side_data(frame, > > AV_FRAME_DATA_VIDEO_HINT); > > + if (mbinfo_sd) { > > + int ret = setup_mb_info(ctx, pic, frame, (const > > AVVideoHint *)mbinfo_sd->data); > > + if (ret < 0) { > > + /* No need to fail here, this is not fatal. We just > > proceed with no > > + * mb_info and log a message */ > > + > > + av_log(ctx, AV_LOG_WARNING, "mb_info setup > > failure\n"); > > + } > > + } > > + > > if (x4->udu_sei) { > > for (int j = 0; j < frame->nb_side_data; j++) { > > AVFrameSideData *side_data = frame->side_data[j]; > > @@ -1102,6 +1179,9 @@ FF_ENABLE_DEPRECATION_WARNINGS > > } > > } > > > > + x4->params.analyse.b_mb_info = x4->mb_info; > > + x4->params.analyse.b_fast_pskip = 1; > > + > > // update AVCodecContext with x264 parameters > > avctx->has_b_frames = x4->params.i_bframe ? > > x4->params.i_bframe_pyramid ? 2 : 1 : 0; > > @@ -1311,6 +1391,7 @@ static const AVOption options[] = { > > { "noise_reduction", "Noise > > reduction", OFFSET(noise_reduction), > > AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE }, > > { "udu_sei", "Use user data unregistered SEI if > > available", OFFSET(udu_sei), AV_OPT_TYPE_BOOL, { .i64 = 0 > > }, 0, 1, VE }, > > { "x264-params", "Override the x264 configuration using a :- > > separated list of key=value parameters", OFFSET(x264_params), > > AV_OPT_TYPE_DICT, { 0 }, 0, 0, VE }, > > + { "mb_info", "Set mb_info data through AVSideData, only > > useful when used from the API", OFFSET(mb_info), AV_OPT_TYPE_BOOL, > > { .i64 = 0 }, 0, 1, VE }, > > { NULL }, > > }; > > > > diff --git a/libavutil/Makefile b/libavutil/Makefile > > index dc9012f9a8..bb5ecc3235 100644 > > --- a/libavutil/Makefile > > +++ b/libavutil/Makefile > > @@ -91,6 +91,7 @@ HEADERS = > > adler32.h \ > > > > tea.h \ > > > > tx.h \ > > > > film_grain_params.h \ > > + > > video_hint_info.h > > \ > > > > > ARCH_HEADERS = > > bswap.h \ > > > > intmath.h \ > > @@ -196,6 +197,7 @@ OBJS-$(CONFIG_VAAPI) += > > hwcontext_vaapi.o > > OBJS-$(CONFIG_VIDEOTOOLBOX) += > > hwcontext_videotoolbox.o > > OBJS-$(CONFIG_VDPAU) += hwcontext_vdpau.o > > OBJS-$(CONFIG_VULKAN) += hwcontext_vulkan.o > > +OBJS-$(CONFIG_LIBX264) += video_hint_info.o > > order > > > > > OBJS-$(!CONFIG_VULKAN) += hwcontext_stub.o > > > > @@ -219,6 +221,8 @@ SKIPHEADERS-$(CONFIG_VULKAN) += > > hwcontext_vulkan.h vulkan.h \ > > > > vulkan_functions.h \ > > vulkan_loader.h > > > > +SKIPHEADERS-$(CONFIG_LIBX264) += video_hint_info.h > > + > > TESTPROGS = > > adler32 \ > > > > aes \ > > > > aes_ctr \ > > diff --git a/libavutil/frame.h b/libavutil/frame.h > > index a491315f25..0e765e5499 100644 > > --- a/libavutil/frame.h > > +++ b/libavutil/frame.h > > @@ -214,6 +214,16 @@ enum AVFrameSideDataType { > > * Ambient viewing environment metadata, as defined by H.274. > > */ > > AV_FRAME_DATA_AMBIENT_VIEWING_ENVIRONMENT, > > + > > + /** > > + * Provide macro block encoder-specific hinting information > > for the encoder > > + * processing. It can be used to pass information about which > > macroblock > > + * can be skipped because it hasn't changed from the > > corresponding one in > > + * the previous frame. This is useful for applications which > > know in > > + * advance this information to speed up real-time encoding. > > Currently only > > + * used by libx264. > > + */ > > + AV_FRAME_DATA_VIDEO_HINT, > > }; > > > > enum AVActiveFormatDescription { > > diff --git a/libavutil/video_hint_info.c > > b/libavutil/video_hint_info.c > > new file mode 100644 > > index 0000000000..0a763962da > > --- /dev/null > > +++ b/libavutil/video_hint_info.c > > @@ -0,0 +1,89 @@ > > +/* > > + * Copyright 2023 Elias Carotti <eliascrt at amazon dot it> > > + * > > + * This file is part of FFmpeg. > > + * > > + * FFmpeg is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later > > version. > > + * > > + * FFmpeg is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General > > Public > > + * License along with FFmpeg; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > > 02110-1301 USA > > + */ > > + > > +#include <string.h> > > + > > +#include "avstring.h" > > +#include "frame.h" > > +#include "macros.h" > > +#include "mem.h" > > +#include "video_hint_info.h" > > + > > +AVVideoHint *av_video_hint_alloc(AVVideoRect *rects, > > > + size_t nb_rects, > > + VideoHintType type, > > + size_t* out_size) > > align to ( > > > +{ > > + struct TestStruct { > > + AVVideoHint p; > > + AVVideoRect b; > > + }; > > nit: weird align (also use more meaningful names - e.g. hint and > rect) > > > + const size_t blocks_offset = offsetof(struct TestStruct, b); > > + size_t size = blocks_offset; > > > + AVVideoHint *par; > > AVVideoHint *hint? > > > + > > + *out_size = 0; > > > + if (nb_rects > (SIZE_MAX - size) / sizeof(AVVideoRect)) > > why SIZE_MAX - size? > > > + return NULL; > > > + size += sizeof(AVVideoRect) * nb_rects; > > + > > + par = av_mallocz(size); > > + if (!par) > > + return NULL; > > + > > + par->type = type; > > + par->nb_rects = nb_rects; > > + par->blocks_offset = blocks_offset; > > + > > + /* Just copies the rects over the newly allocated buffer */ > > + memcpy((uint8_t *)par + blocks_offset, rects, > > sizeof(AVVideoRect) * nb_rects); > > + > > + *out_size = size; > > + > > + return par; > > +} > > + > > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > > + AVVideoRect *rects, > > + size_t num_rects, > > + VideoHintType type) > > +{ > > > + AVVideoHint *par; > > use a more meaningful name (I think "par" is a leftover of an older > API) > > > + AVBufferRef *buf; > > + size_t size = 0; > > + > > + par = av_video_hint_alloc(rects, num_rects, type, &size); > > + if (!par) > > + return NULL; > > + > > + buf = av_buffer_create((uint8_t *)par, size, NULL, NULL, 0); > > + if (!buf) { > > + av_freep(&par); > > + return NULL; > > + } > > + > > + if (!av_frame_new_side_data_from_buf(frame, > > AV_FRAME_DATA_VIDEO_HINT, buf)) { > > + av_buffer_unref(&buf); > > + return NULL; > > + } > > + > > + return par; > > +} > > + > > diff --git a/libavutil/video_hint_info.h > > b/libavutil/video_hint_info.h > > new file mode 100644 > > index 0000000000..3b04e0c40e > > --- /dev/null > > +++ b/libavutil/video_hint_info.h > > @@ -0,0 +1,87 @@ > > +/** > > + * Copyright 2023 Elias Carotti <eliascrt at amazon dot it> > > + * > > + * This file is part of FFmpeg. > > + * > > + * FFmpeg is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later > > version. > > + * > > + * FFmpeg is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General > > Public > > + * License along with FFmpeg; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > > 02110-1301 USA > > + */ > > + > > +#ifndef AVUTIL_VIDEO_HINT_INFO_H > > +#define AVUTIL_VIDEO_HINT_INFO_H > > + > > +#include <stddef.h> > > +#include <stdint.h> > > +#include "libavutil/avassert.h" > > +#include "libavutil/frame.h" > > + > > +typedef struct AVVideoRect { > > + uint32_t x, y; > > + uint32_t width, height; > > +} AVVideoRect; > > + > > > +typedef enum VideoHintType { > > + /* blocks delimit the constant areas (unchanged), default is > > changed */ > > + AV_VIDEO_HINT_CONSTANT, > > + > > + /* blocks delimit the constant areas (changed), default is not > > changed */ > > + AV_VIDEO_HINT_CHANGED, > > +} VideoHintType; > > missing AV prefix > > > + > > +typedef struct AVVideoHint { > > + /** > > + * Number of blocks in the array. > > + * > > + * May be 0, in which case no per-block information is > > present. In this case > > + * the values of blocks_offset / block_size are unspecified > > and should not > > + * be accessed. > > + */ > > + int nb_rects; > > + > > + /** > > + * Offset in bytes from the beginning of this structure at > > which the array > > + * of blocks starts. > > + */ > > + size_t blocks_offset; > > + > > + VideoHintType type; > > +} AVVideoHint; > > + > > +static av_always_inline AVVideoRect* > > +av_video_hint_rects(const AVVideoHint *par) > > +{ > > + return (AVVideoRect *)((uint8_t *)par + par->blocks_offset); > > +} > > + > > +/** > > + * Allocate memory for a vector of AVVideoRect in the given > > AVFrame > > + * {@code frame} as AVFrameSideData of type > > AV_FRAME_DATA_VIDEO_HINT_INFO. > > + * The side data contains a list of rectangles for the portions of > > the frame > > + * which changed from the last encoded one (and the remainder are > > assumed to be > > + * changed), or, alternately (depending on the type parameter) the > > unchanged > > + * ones (and the remanining ones are those which changed). > > + * Macroblocks will thus be hinted either to be P_SKIP-ped or go > > through the > > + * regular encoding procedure. > > + */ > > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > > + AVVideoRect *rects, > > + size_t num_rects, > > + VideoHintType type); > > + > > +AVVideoHint *av_video_hint_alloc(AVVideoRect *rects, > > + size_t nb_rects, > > + VideoHintType type, > > + size_t *out_size); > > + > > > +#endif /* AVUTIL__VIDEO_HINT_INFO_H */ > > AVUTIL_VIDEO... > > > -- > > 2.34.1 > > > > > _______________________________________________ > > 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". > > _______________________________________________ > 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". NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On date Monday 2023-06-05 15:32:35 +0000, Carotti, Elias wrote: > Hi, > please find attached the patch which I updated according to your > suggestions. > Best, > Elias [...] > From 8288d2bd36ffed29140d46c42b6f5515a9058836 Mon Sep 17 00:00:00 2001 > From: Elias Carotti <eliascr _at_ amazon _dot_ it> > Date: Wed, 19 Apr 2023 11:49:39 +0200 > Subject: [PATCH] Add support for libx264's MB_INFO > > libx264's x264_image_properties_t, which is passed to the encoding function, > contains a field to pass down information on the portions of the frame which > changed with respect to the previous one (used for prediction) to mark > unchanged macroblocks P_SKIP. > --- > libavcodec/libx264.c | 94 +++++++++++++++++++++++++++++++++++++ > libavutil/Makefile | 4 ++ > libavutil/frame.h | 10 ++++ > libavutil/video_hint_info.c | 89 +++++++++++++++++++++++++++++++++++ > libavutil/video_hint_info.h | 87 ++++++++++++++++++++++++++++++++++ > 5 files changed, 284 insertions(+) > create mode 100644 libavutil/video_hint_info.c > create mode 100644 libavutil/video_hint_info.h > > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c > index 5736f1efa7..2cf7755eec 100644 > --- a/libavcodec/libx264.c > +++ b/libavcodec/libx264.c > @@ -30,6 +30,7 @@ > #include "libavutil/stereo3d.h" > #include "libavutil/time.h" > #include "libavutil/intreadwrite.h" > +#include "libavutil/video_hint_info.h" > #include "avcodec.h" > #include "codec_internal.h" > #include "encode.h" > @@ -48,6 +49,13 @@ > // from x264.h, for quant_offsets, Macroblocks are 16x16 > // blocks of pixels (with respect to the luma plane) > #define MB_SIZE 16 > +#define MB_LSIZE 4 > +#define MB_FLOOR(x) ((x) >> (MB_LSIZE)) > +#define MB_CEIL(x) MB_FLOOR((x) + (MB_SIZE - 1)) > + > +typedef void (*AVMBInfoComputeCoords)(const AVVideoRect *rect, > + int *min_x, int *max_x, > + int *min_y, int *max_y); > > typedef struct X264Opaque { > #if FF_API_REORDERED_OPAQUE > @@ -123,6 +131,8 @@ typedef struct X264Context { > * encounter a frame with ROI side data. > */ > int roi_warned; > + > + int mb_info; > } X264Context; > > static void X264_log(void *p, int level, const char *fmt, va_list args) > @@ -295,6 +305,7 @@ static void free_picture(x264_picture_t *pic) > av_free(pic->extra_sei.payloads[i].payload); > av_freep(&pic->extra_sei.payloads); > av_freep(&pic->prop.quant_offsets); > + av_freep(&pic->prop.mb_info); > pic->extra_sei.num_payloads = 0; > } > > @@ -320,6 +331,73 @@ static enum AVPixelFormat csp_to_pixfmt(int csp) > return AV_PIX_FMT_NONE; > } > > +static void mbinfo_compute_changed_coords(const AVVideoRect *rect, > + int *min_x, > + int *max_x, > + int *min_y, > + int *max_y) > +{ > + *min_y = MB_FLOOR(rect->y); > + *max_y = MB_CEIL(rect->y + rect->height); > + *min_x = MB_FLOOR(rect->x); > + *max_x = MB_CEIL(rect->x + rect->width); > +} > + > +static void mbinfo_compute_constant_coords(const AVVideoRect *rect, > + int *min_x, > + int *max_x, > + int *min_y, > + int *max_y) > +{ > + *min_y = MB_CEIL(rect->y); > + *max_y = MB_FLOOR(rect->y + rect->height); > + *min_x = MB_CEIL(rect->x); > + *max_x = MB_FLOOR(rect->x + rect->width); > +} > + > +static int setup_mb_info(AVCodecContext *ctx, x264_picture_t *pic, > + const AVFrame *frame, > + const AVVideoHint *info) > +{ > + int mb_width = (frame->width + MB_SIZE - 1) / MB_SIZE; > + int mb_height = (frame->height + MB_SIZE - 1) / MB_SIZE; > + int mbinfo_filler; > + int mbinfo_marker; > + AVMBInfoComputeCoords compute_coords_fn; > + > + const AVVideoRect *mbinfo_rects; > + int nb_rects; > + uint8_t *mbinfo; > + > + mbinfo_rects = (const AVVideoRect *)av_video_hint_rects(info); > + nb_rects = info->nb_rects; > + > + mbinfo = av_calloc(mb_width * mb_height, sizeof(*mbinfo)); > + if (!mbinfo) > + return AVERROR(ENOMEM); > + > + mbinfo_filler = (info->type == AV_VIDEO_HINT_CHANGED) ? X264_MBINFO_CONSTANT : 0; > + mbinfo_marker = (info->type == AV_VIDEO_HINT_CHANGED) ? 0 : X264_MBINFO_CONSTANT; > + compute_coords_fn = (info->type == AV_VIDEO_HINT_CHANGED) ? mbinfo_compute_changed_coords : mbinfo_compute_constant_coords; > + > + memset(mbinfo, mbinfo_filler, sizeof(*mbinfo) * mb_width * mb_height); > + for (int i = 0; i < nb_rects; i++) { > + int min_x, max_x, min_y, max_y; > + > + (*compute_coords_fn)(mbinfo_rects, &min_x, &max_x, &min_y, &max_y); > + for (int mb_y = min_y; mb_y < max_y; ++mb_y) { > + memset(mbinfo + mb_y * mb_width + min_x, mbinfo_marker, max_x - min_x); > + } > + > + mbinfo_rects++; > + } maybe #define COMPUTE_MBINFO(mbinfo_filler_, mbinfo_marker_, compute_coords_fn_) \ memset(mbinfo, mbinfo_filler_, sizeof(*mbinfo) * mb_width * mb_height); \ \ for (int i = 0; i < nb_rects; i++) { \ int min_x, max_x, min_y, max_y; \ \ compute_coords_fn_(mbinfo_rects, &min_x, &max_x, &min_y, &max_y); \ for (int mb_y = min_y; mb_y < max_y; ++mb_y) { \ memset(mbinfo + mb_y * mb_width + min_x, mbinfo_marker_, max_x - min_x); \ } \ \ mbinfo_rects++; \ } \ if (info->type == AV_VIDEO_HINT_CHANGED) { COMPUTE_MBINFO(X264_MBINFO_CONSTANT, 0, mbinfo_compute_changed_coords); } else /* if (info->type == AV_VIDEO_HINT_CHANGED) */ { COMPUTE_MBINFO(0, X264_MBINFO_CONSTANT, mbinfo_compute_constant_coords); } this adds to spatial complexity but enables the use of inlined functions to avoid the function call in the loop > + pic->prop.mb_info = mbinfo; > + pic->prop.mb_info_free = av_free; > + > + return 0; > +} > + > static int setup_roi(AVCodecContext *ctx, x264_picture_t *pic, int bit_depth, > const AVFrame *frame, const uint8_t *data, size_t size) > { > @@ -404,6 +482,7 @@ static int setup_frame(AVCodecContext *ctx, const AVFrame *frame, > int64_t wallclock = 0; > int bit_depth, ret; > AVFrameSideData *sd; > + AVFrameSideData *mbinfo_sd; > > *ppic = NULL; > if (!frame) > @@ -499,6 +578,17 @@ FF_ENABLE_DEPRECATION_WARNINGS > goto fail; > } > > + mbinfo_sd = av_frame_get_side_data(frame, AV_FRAME_DATA_VIDEO_HINT); > + if (mbinfo_sd) { > + int ret = setup_mb_info(ctx, pic, frame, (const AVVideoHint *)mbinfo_sd->data); > + if (ret < 0) { > + /* No need to fail here, this is not fatal. We just proceed with no > + * mb_info and log a message */ > + > + av_log(ctx, AV_LOG_WARNING, "mb_info setup failure\n"); nit to provide more context: "setup_mb_info failed with error: %s\n", av_strerror(ret) > + } > + } > + > if (x4->udu_sei) { > for (int j = 0; j < frame->nb_side_data; j++) { > AVFrameSideData *side_data = frame->side_data[j]; > @@ -1102,6 +1192,9 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > } > > + x4->params.analyse.b_mb_info = x4->mb_info; > + x4->params.analyse.b_fast_pskip = 1; > + > // update AVCodecContext with x264 parameters > avctx->has_b_frames = x4->params.i_bframe ? > x4->params.i_bframe_pyramid ? 2 : 1 : 0; > @@ -1311,6 +1404,7 @@ static const AVOption options[] = { > { "noise_reduction", "Noise reduction", OFFSET(noise_reduction), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE }, > { "udu_sei", "Use user data unregistered SEI if available", OFFSET(udu_sei), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, > { "x264-params", "Override the x264 configuration using a :-separated list of key=value parameters", OFFSET(x264_params), AV_OPT_TYPE_DICT, { 0 }, 0, 0, VE }, > + { "mb_info", "Set mb_info data through AVSideData, only useful when used from the API", OFFSET(mb_info), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, > { NULL }, > }; [...] > enum AVActiveFormatDescription { > diff --git a/libavutil/video_hint_info.c b/libavutil/video_hint_info.c > new file mode 100644 > index 0000000000..c920bd6232 > --- /dev/null > +++ b/libavutil/video_hint_info.c > @@ -0,0 +1,89 @@ > +/* > + * Copyright 2023 Elias Carotti <eliascrt at amazon dot it> > + * > + * This file is part of FFmpeg. > + * > + * FFmpeg is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * FFmpeg is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + */ > + > +#include <string.h> > + > +#include "avstring.h" > +#include "frame.h" > +#include "macros.h" > +#include "mem.h" > +#include "video_hint_info.h" > + > +AVVideoHint *av_video_hint_alloc(AVVideoRect *rects, > + size_t nb_rects, > + AVVideoHintType type, > + size_t* out_size) > +{ > + struct TestStruct { > + AVVideoHint hint; > + AVVideoRect rect; > + }; > + const size_t blocks_offset = offsetof(struct TestStruct, rect); > + size_t size = blocks_offset; > + AVVideoHint *hint; > + > + *out_size = 0; > + if (nb_rects > (SIZE_MAX - size) / sizeof(AVVideoRect)) > + return NULL; > + size += sizeof(AVVideoRect) * nb_rects; > + > + hint = av_mallocz(size); > + if (!hint) > + return NULL; > + > + hint->type = type; > + hint->nb_rects = nb_rects; > + hint->blocks_offset = blocks_offset; > + > + /* Just copies the rects over the newly allocated buffer */ > + memcpy((uint8_t *)hint + blocks_offset, rects, sizeof(AVVideoRect) * nb_rects); > + > + *out_size = size; > + > + return hint; > +} > + > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > + AVVideoRect *rects, > + size_t num_rects, > + AVVideoHintType type) > +{ > + AVVideoHint *hint; > + AVBufferRef *buf; > + size_t size = 0; > + > + hint = av_video_hint_alloc(rects, num_rects, type, &size); > + if (!hint) > + return NULL; > + > + buf = av_buffer_create((uint8_t *)hint, size, NULL, NULL, 0); > + if (!buf) { > + av_freep(&hint); > + return NULL; > + } > + > + if (!av_frame_new_side_data_from_buf(frame, AV_FRAME_DATA_VIDEO_HINT, buf)) { > + av_buffer_unref(&buf); > + return NULL; > + } > + > + return hint; > +} > + > diff --git a/libavutil/video_hint_info.h b/libavutil/video_hint_info.h > new file mode 100644 > index 0000000000..2844398d18 > --- /dev/null > +++ b/libavutil/video_hint_info.h strip the _info part since to have simple mapping between data structure and file names [...] Looks good to me otherwise, maybe Michael/Anton or someone else want to have a look?
> > Looks good to me otherwise, maybe Michael/Anton or someone else want > to have a look? > I don't think we should be adding what is essentially libx264 specific code to the public libavutil API. Kieran
Hi Stefano, Here is the revised patch according to your suggestions. This should allow for efficient inlining of the methods computing the map of skipped macrobloks. Best, Elias On Sun, 2023-06-11 at 19:15 +0200, Stefano Sabatini wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > On date Monday 2023-06-05 15:32:35 +0000, Carotti, Elias wrote: > > Hi, > > please find attached the patch which I updated according to your > > suggestions. > > Best, > > Elias > [...] > > > From 8288d2bd36ffed29140d46c42b6f5515a9058836 Mon Sep 17 00:00:00 > > 2001 > > From: Elias Carotti <eliascr _at_ amazon _dot_ it> > > Date: Wed, 19 Apr 2023 11:49:39 +0200 > > Subject: [PATCH] Add support for libx264's MB_INFO > > > > libx264's x264_image_properties_t, which is passed to the encoding > > function, > > contains a field to pass down information on the portions of the > > frame which > > changed with respect to the previous one (used for prediction) to > > mark > > unchanged macroblocks P_SKIP. > > --- > > libavcodec/libx264.c | 94 > > +++++++++++++++++++++++++++++++++++++ > > libavutil/Makefile | 4 ++ > > libavutil/frame.h | 10 ++++ > > libavutil/video_hint_info.c | 89 > > +++++++++++++++++++++++++++++++++++ > > libavutil/video_hint_info.h | 87 > > ++++++++++++++++++++++++++++++++++ > > 5 files changed, 284 insertions(+) > > create mode 100644 libavutil/video_hint_info.c > > create mode 100644 libavutil/video_hint_info.h > > > > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c > > index 5736f1efa7..2cf7755eec 100644 > > --- a/libavcodec/libx264.c > > +++ b/libavcodec/libx264.c > > @@ -30,6 +30,7 @@ > > #include "libavutil/stereo3d.h" > > #include "libavutil/time.h" > > #include "libavutil/intreadwrite.h" > > +#include "libavutil/video_hint_info.h" > > #include "avcodec.h" > > #include "codec_internal.h" > > #include "encode.h" > > @@ -48,6 +49,13 @@ > > // from x264.h, for quant_offsets, Macroblocks are 16x16 > > // blocks of pixels (with respect to the luma plane) > > #define MB_SIZE 16 > > +#define MB_LSIZE 4 > > +#define MB_FLOOR(x) ((x) >> (MB_LSIZE)) > > +#define MB_CEIL(x) MB_FLOOR((x) + (MB_SIZE - 1)) > > + > > +typedef void (*AVMBInfoComputeCoords)(const AVVideoRect *rect, > > + int *min_x, int *max_x, > > + int *min_y, int *max_y); > > > > typedef struct X264Opaque { > > #if FF_API_REORDERED_OPAQUE > > @@ -123,6 +131,8 @@ typedef struct X264Context { > > * encounter a frame with ROI side data. > > */ > > int roi_warned; > > + > > + int mb_info; > > } X264Context; > > > > static void X264_log(void *p, int level, const char *fmt, va_list > > args) > > @@ -295,6 +305,7 @@ static void free_picture(x264_picture_t *pic) > > av_free(pic->extra_sei.payloads[i].payload); > > av_freep(&pic->extra_sei.payloads); > > av_freep(&pic->prop.quant_offsets); > > + av_freep(&pic->prop.mb_info); > > pic->extra_sei.num_payloads = 0; > > } > > > > @@ -320,6 +331,73 @@ static enum AVPixelFormat csp_to_pixfmt(int > > csp) > > return AV_PIX_FMT_NONE; > > } > > > > +static void mbinfo_compute_changed_coords(const AVVideoRect *rect, > > + int *min_x, > > + int *max_x, > > + int *min_y, > > + int *max_y) > > +{ > > + *min_y = MB_FLOOR(rect->y); > > + *max_y = MB_CEIL(rect->y + rect->height); > > + *min_x = MB_FLOOR(rect->x); > > + *max_x = MB_CEIL(rect->x + rect->width); > > +} > > + > > +static void mbinfo_compute_constant_coords(const AVVideoRect > > *rect, > > + int *min_x, > > + int *max_x, > > + int *min_y, > > + int *max_y) > > +{ > > + *min_y = MB_CEIL(rect->y); > > + *max_y = MB_FLOOR(rect->y + rect->height); > > + *min_x = MB_CEIL(rect->x); > > + *max_x = MB_FLOOR(rect->x + rect->width); > > +} > > + > > +static int setup_mb_info(AVCodecContext *ctx, x264_picture_t *pic, > > + const AVFrame *frame, > > + const AVVideoHint *info) > > +{ > > + int mb_width = (frame->width + MB_SIZE - 1) / MB_SIZE; > > + int mb_height = (frame->height + MB_SIZE - 1) / MB_SIZE; > > + int mbinfo_filler; > > + int mbinfo_marker; > > + AVMBInfoComputeCoords compute_coords_fn; > > + > > + const AVVideoRect *mbinfo_rects; > > + int nb_rects; > > + uint8_t *mbinfo; > > + > > + mbinfo_rects = (const AVVideoRect *)av_video_hint_rects(info); > > + nb_rects = info->nb_rects; > > + > > + mbinfo = av_calloc(mb_width * mb_height, sizeof(*mbinfo)); > > + if (!mbinfo) > > + return AVERROR(ENOMEM); > > + > > > + mbinfo_filler = (info->type == AV_VIDEO_HINT_CHANGED) ? > > X264_MBINFO_CONSTANT : 0; > > + mbinfo_marker = (info->type == AV_VIDEO_HINT_CHANGED) ? 0 > > : X264_MBINFO_CONSTANT; > > + compute_coords_fn = (info->type == AV_VIDEO_HINT_CHANGED) ? > > mbinfo_compute_changed_coords : mbinfo_compute_constant_coords; > > + > > + memset(mbinfo, mbinfo_filler, sizeof(*mbinfo) * mb_width * > > mb_height); > > + for (int i = 0; i < nb_rects; i++) { > > + int min_x, max_x, min_y, max_y; > > + > > + (*compute_coords_fn)(mbinfo_rects, &min_x, &max_x, &min_y, > > &max_y); > > + for (int mb_y = min_y; mb_y < max_y; ++mb_y) { > > + memset(mbinfo + mb_y * mb_width + min_x, > > mbinfo_marker, max_x - min_x); > > + } > > + > > + mbinfo_rects++; > > + } > > maybe > > #define COMPUTE_MBINFO(mbinfo_filler_, mbinfo_marker_, > compute_coords_fn_) \ > memset(mbinfo, mbinfo_filler_, sizeof(*mbinfo) * mb_width * > mb_height); \ > > \ > for (int i = 0; i < nb_rects; i++) > { \ > int min_x, max_x, min_y, > max_y; \ > > \ > compute_coords_fn_(mbinfo_rects, &min_x, &max_x, &min_y, > &max_y); \ > for (int mb_y = min_y; mb_y < max_y; ++mb_y) > { \ > memset(mbinfo + mb_y * mb_width + min_x, mbinfo_marker_, > max_x - min_x); \ > > } \ > > \ > > mbinfo_rects++; \ > > } \ > > if (info->type == AV_VIDEO_HINT_CHANGED) { > COMPUTE_MBINFO(X264_MBINFO_CONSTANT, 0, > mbinfo_compute_changed_coords); > } else /* if (info->type == AV_VIDEO_HINT_CHANGED) */ { > COMPUTE_MBINFO(0, X264_MBINFO_CONSTANT, > mbinfo_compute_constant_coords); > } > > this adds to spatial complexity but enables the use of inlined > functions to avoid the function call in the loop > > > + pic->prop.mb_info = mbinfo; > > + pic->prop.mb_info_free = av_free; > > + > > + return 0; > > +} > > + > > static int setup_roi(AVCodecContext *ctx, x264_picture_t *pic, int > > bit_depth, > > const AVFrame *frame, const uint8_t *data, > > size_t size) > > { > > @@ -404,6 +482,7 @@ static int setup_frame(AVCodecContext *ctx, > > const AVFrame *frame, > > int64_t wallclock = 0; > > int bit_depth, ret; > > AVFrameSideData *sd; > > + AVFrameSideData *mbinfo_sd; > > > > *ppic = NULL; > > if (!frame) > > @@ -499,6 +578,17 @@ FF_ENABLE_DEPRECATION_WARNINGS > > goto fail; > > } > > > > + mbinfo_sd = av_frame_get_side_data(frame, > > AV_FRAME_DATA_VIDEO_HINT); > > + if (mbinfo_sd) { > > + int ret = setup_mb_info(ctx, pic, frame, (const > > AVVideoHint *)mbinfo_sd->data); > > + if (ret < 0) { > > + /* No need to fail here, this is not fatal. We just > > proceed with no > > + * mb_info and log a message */ > > + > > > + av_log(ctx, AV_LOG_WARNING, "mb_info setup > > failure\n"); > > nit to provide more context: "setup_mb_info failed with error: %s\n", > av_strerror(ret) > > > + } > > + } > > + > > if (x4->udu_sei) { > > for (int j = 0; j < frame->nb_side_data; j++) { > > AVFrameSideData *side_data = frame->side_data[j]; > > @@ -1102,6 +1192,9 @@ FF_ENABLE_DEPRECATION_WARNINGS > > } > > } > > > > + x4->params.analyse.b_mb_info = x4->mb_info; > > + x4->params.analyse.b_fast_pskip = 1; > > + > > // update AVCodecContext with x264 parameters > > avctx->has_b_frames = x4->params.i_bframe ? > > x4->params.i_bframe_pyramid ? 2 : 1 : 0; > > @@ -1311,6 +1404,7 @@ static const AVOption options[] = { > > { "noise_reduction", "Noise > > reduction", OFFSET(noise_reduction), > > AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE }, > > { "udu_sei", "Use user data unregistered SEI if > > available", OFFSET(udu_sei), AV_OPT_TYPE_BOOL, { .i64 = 0 > > }, 0, 1, VE }, > > { "x264-params", "Override the x264 configuration using a :- > > separated list of key=value parameters", OFFSET(x264_params), > > AV_OPT_TYPE_DICT, { 0 }, 0, 0, VE }, > > + { "mb_info", "Set mb_info data through AVSideData, only > > useful when used from the API", OFFSET(mb_info), AV_OPT_TYPE_BOOL, > > { .i64 = 0 }, 0, 1, VE }, > > { NULL }, > > }; > [...] > > enum AVActiveFormatDescription { > > diff --git a/libavutil/video_hint_info.c > > b/libavutil/video_hint_info.c > > new file mode 100644 > > index 0000000000..c920bd6232 > > --- /dev/null > > +++ b/libavutil/video_hint_info.c > > @@ -0,0 +1,89 @@ > > +/* > > + * Copyright 2023 Elias Carotti <eliascrt at amazon dot it> > > + * > > + * This file is part of FFmpeg. > > + * > > + * FFmpeg is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later > > version. > > + * > > + * FFmpeg is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General > > Public > > + * License along with FFmpeg; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > > 02110-1301 USA > > + */ > > + > > +#include <string.h> > > + > > +#include "avstring.h" > > +#include "frame.h" > > +#include "macros.h" > > +#include "mem.h" > > +#include "video_hint_info.h" > > + > > +AVVideoHint *av_video_hint_alloc(AVVideoRect *rects, > > + size_t nb_rects, > > + AVVideoHintType type, > > + size_t* out_size) > > +{ > > + struct TestStruct { > > + AVVideoHint hint; > > + AVVideoRect rect; > > + }; > > > + const size_t blocks_offset = offsetof(struct TestStruct, > > rect); > > + size_t size = blocks_offset; > > + AVVideoHint *hint; > > + > > + *out_size = 0; > > + if (nb_rects > (SIZE_MAX - size) / sizeof(AVVideoRect)) > > + return NULL; > > + size += sizeof(AVVideoRect) * nb_rects; > > + > > + hint = av_mallocz(size); > > + if (!hint) > > + return NULL; > > + > > + hint->type = type; > > + hint->nb_rects = nb_rects; > > + hint->blocks_offset = blocks_offset; > > + > > + /* Just copies the rects over the newly allocated buffer */ > > + memcpy((uint8_t *)hint + blocks_offset, rects, > > sizeof(AVVideoRect) * nb_rects); > > + > > + *out_size = size; > > + > > + return hint; > > +} > > + > > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > > + AVVideoRect *rects, > > + size_t num_rects, > > + AVVideoHintType type) > > +{ > > + AVVideoHint *hint; > > + AVBufferRef *buf; > > + size_t size = 0; > > + > > + hint = av_video_hint_alloc(rects, num_rects, type, &size); > > + if (!hint) > > + return NULL; > > + > > + buf = av_buffer_create((uint8_t *)hint, size, NULL, NULL, 0); > > + if (!buf) { > > + av_freep(&hint); > > + return NULL; > > + } > > + > > + if (!av_frame_new_side_data_from_buf(frame, > > AV_FRAME_DATA_VIDEO_HINT, buf)) { > > + av_buffer_unref(&buf); > > + return NULL; > > + } > > + > > + return hint; > > +} > > + > > diff --git a/libavutil/video_hint_info.h > > b/libavutil/video_hint_info.h > > new file mode 100644 > > index 0000000000..2844398d18 > > --- /dev/null > > > +++ b/libavutil/video_hint_info.h > > strip the _info part since to have simple mapping between data > structure and file names > > [...] > > Looks good to me otherwise, maybe Michael/Anton or someone else want > to have a look? > _______________________________________________ > 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". NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On Mon, 2023-06-12 at 18:23 +1000, Kieran Kunhya wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > > > > Looks good to me otherwise, maybe Michael/Anton or someone else > > want > > to have a look? > > > > I don't think we should be adding what is essentially libx264 > specific code > to the public libavutil API. > > Kieran Hi Kieran, I disagree: this is not more libx264-specific than most other code in libavutil/libx264.c, like, say, the region of interest code. P_SKIPs are in the standard and this API is designed to provide a generic way to generate them when the changing portion of a frame is known in advance. Should other encoders (and I mean it in the broadest sense, not specifically h.264 ones) support a method for feeding in hints about unchanged portions of a frame, it shouldn't be hard to write the encoder-specific code for them in the respective encoder-specific file. In fact, the public api provided only allows to specify as side data a list of rectangles (in pixels' space) on the frame (just like it's done for the region of interest) which is then translated into the specific libx264 hinting mechanism (in macroblock space.) Best, Elias > _______________________________________________ > 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". NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On date Monday 2023-06-12 17:38:43 +0000, Carotti, Elias wrote: > On Mon, 2023-06-12 at 18:23 +1000, Kieran Kunhya wrote: > > CAUTION: This email originated from outside of the organization. Do > > not click links or open attachments unless you can confirm the sender > > and know the content is safe. > > > > > > > > Looks good to me otherwise, maybe Michael/Anton or someone else > > > want > > > to have a look? > > > > > > > I don't think we should be adding what is essentially libx264 > > specific code > > to the public libavutil API. > > > > Kieran > > Hi Kieran, > I disagree: this is not more libx264-specific than most other code in > libavutil/libx264.c, like, say, the region of interest code. > > P_SKIPs are in the standard and this API is designed to provide a > generic way to generate them when the changing portion of a frame is > known in advance. > > Should other encoders (and I mean it in the broadest sense, not > specifically h.264 ones) support a method for feeding in hints about > unchanged portions of a frame, it shouldn't be hard to write the > encoder-specific code for them in the respective encoder-specific file. > > In fact, the public api provided only allows to specify as side data a > list of rectangles (in pixels' space) on the frame (just like it's done > for the region of interest) which is then translated into the specific > libx264 hinting mechanism (in macroblock space.) +1, I don't think there is anything specific for x264 here. In fact, we could even add a filter to compute the hinting data and export it in the side data. If you think this is a good idea, we could split the patches in two parts to make this more apparent.
On date Monday 2023-06-12 17:28:10 +0000, Carotti, Elias wrote: > Hi Stefano, > Here is the revised patch according to your suggestions. This should > allow for efficient inlining of the methods computing the map of > skipped macrobloks. > Best, > Elias [...] > > From 0e7979250231edbe0b845cee96c473bc6e07d46b Mon Sep 17 00:00:00 2001 > From: Elias Carotti <eliascrt _at_ amazon _dot_ it> > Date: Wed, 19 Apr 2023 11:49:39 +0200 > Subject: [PATCH] Add support for libx264's MB_INFO > > libx264's x264_image_properties_t, which is passed to the encoding function, > contains a field to pass down information on the portions of the frame which > changed with respect to the previous one (used for prediction) to mark > unchanged macroblocks P_SKIP. > --- > libavcodec/libx264.c | 91 ++++++++++++++++++++++++++++++++++++++++++ > libavutil/Makefile | 4 ++ > libavutil/frame.h | 10 +++++ > libavutil/video_hint.c | 89 +++++++++++++++++++++++++++++++++++++++++ > libavutil/video_hint.h | 87 ++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 281 insertions(+) > create mode 100644 libavutil/video_hint.c > create mode 100644 libavutil/video_hint.h Note: maybe this should be split in two parts, one about libavutil and one about libx264. The libavutil part should also contain a minor bump for the new feature, same for libavcodec (with corresponding changes in doc/APIchanges). I don't know if we track internal dependencies (to define the minor libavutil version needed for the new libx264 feature in libavcodec). What's the current practice here? Looks good to me otherwise (still it would be good if someone else can have a look at it).
On Sun, 2023-06-18 at 12:18 +0200, Stefano Sabatini wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > On date Monday 2023-06-12 17:28:10 +0000, Carotti, Elias wrote: > > Hi Stefano, > > Here is the revised patch according to your suggestions. This > > should > > allow for efficient inlining of the methods computing the map of > > skipped macrobloks. > > Best, > > Elias > [...] > > > > From 0e7979250231edbe0b845cee96c473bc6e07d46b Mon Sep 17 00:00:00 > > 2001 > > From: Elias Carotti <eliascrt _at_ amazon _dot_ it> > > Date: Wed, 19 Apr 2023 11:49:39 +0200 > > Subject: [PATCH] Add support for libx264's MB_INFO > > > > libx264's x264_image_properties_t, which is passed to the encoding > > function, > > contains a field to pass down information on the portions of the > > frame which > > changed with respect to the previous one (used for prediction) to > > mark > > unchanged macroblocks P_SKIP. > > --- > > libavcodec/libx264.c | 91 > > ++++++++++++++++++++++++++++++++++++++++++ > > libavutil/Makefile | 4 ++ > > libavutil/frame.h | 10 +++++ > > libavutil/video_hint.c | 89 > > +++++++++++++++++++++++++++++++++++++++++ > > libavutil/video_hint.h | 87 > > ++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 281 insertions(+) > > create mode 100644 libavutil/video_hint.c > > create mode 100644 libavutil/video_hint.h > > Note: maybe this should be split in two parts, one about libavutil > and > one about libx264. The libavutil part should also contain a minor > bump > for the new feature, same for libavcodec (with corresponding changes > in doc/APIchanges). > > I don't know if we track internal dependencies (to define the minor > libavutil version needed for the new libx264 feature in > libavcodec). What's the current practice here? > > Looks good to me otherwise (still it would be good if someone else > can > have a look at it). .... Hi all, please find the updated patch split into two parts. The first part with the changes to libavutil is appended at the end of this email, while the second patch with the changes to libavcodec will be in a separate mail. Best, Elias NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
Quoting Carotti, Elias (2023-06-21 17:53:09) > + > + /** > + * Provide macro block encoder-specific hinting information for the encoder > + * processing. It can be used to pass information about which macroblock > + * can be skipped because it hasn't changed from the corresponding one in > + * the previous frame. This is useful for applications which know in > + * advance this information to speed up real-time encoding. Currently only > + * used by libx264. I'd avoid any such claims here, because this comment will certainly not be kept up to date. > +/** > + * Allocate memory for a vector of AVVideoRect in the given AVFrame > + * {@code frame} as AVFrameSideData of type AV_FRAME_DATA_VIDEO_HINT_INFO. > + * The side data contains a list of rectangles for the portions of the frame > + * which changed from the last encoded one (and the remainder are assumed to be > + * changed), or, alternately (depending on the type parameter) the unchanged > + * ones (and the remanining ones are those which changed). > + * Macroblocks will thus be hinted either to be P_SKIP-ped or go through the > + * regular encoding procedure. > + */ > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > + AVVideoRect *rects, > + size_t num_rects, > + AVVideoHintType type); > + > +AVVideoHint *av_video_hint_alloc(AVVideoRect *rects, > + size_t nb_rects, > + AVVideoHintType type, > + size_t *out_size); If AVVideoHint is extended in the future, you will have a weird situation where some fields are set by the allocation function, while others have to be set manually by the caller. You're also assuming the caller has an explicit array of AVVideoRect, while they may actually want to fill them in through some other means. Finally, it still seems to me this is largely duplicating AVVideoEncParams and you could get your functionality by adding your AVVideoHintType there.
On Thu, 2023-06-22 at 10:44 +0200, Anton Khirnov wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > Quoting Carotti, Elias (2023-06-21 17:53:09) > > + > > + /** > > + * Provide macro block encoder-specific hinting information > > for the encoder > > + * processing. It can be used to pass information about which > > macroblock > > + * can be skipped because it hasn't changed from the > > corresponding one in > > + * the previous frame. This is useful for applications which > > know in > > + * advance this information to speed up real-time encoding. > > Currently only > > + * used by libx264. > > I'd avoid any such claims here, because this comment will certainly > not > be kept up to date. Agreed. It was more a statement than a claim, since I only implemented that :-) > > > +/** > > + * Allocate memory for a vector of AVVideoRect in the given > > AVFrame > > + * {@code frame} as AVFrameSideData of type > > AV_FRAME_DATA_VIDEO_HINT_INFO. > > + * The side data contains a list of rectangles for the portions of > > the frame > > + * which changed from the last encoded one (and the remainder are > > assumed to be > > + * changed), or, alternately (depending on the type parameter) the > > unchanged > > + * ones (and the remanining ones are those which changed). > > + * Macroblocks will thus be hinted either to be P_SKIP-ped or go > > through the > > + * regular encoding procedure. > > + */ > > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > > + AVVideoRect *rects, > > + size_t num_rects, > > + AVVideoHintType type); > > + > > +AVVideoHint *av_video_hint_alloc(AVVideoRect *rects, > > + size_t nb_rects, > > + AVVideoHintType type, > > + size_t *out_size); > > If AVVideoHint is extended in the future, you will have a weird > situation where some fields are set by the allocation function, while > others have to be set manually by the caller. > > You're also assuming the caller has an explicit array of AVVideoRect, > while they may actually want to fill them in through some other > means. I agree on the first issue, and yes, it would be wiser to split the allocation function from a the setting function. Would a simple append_rectangles (the name may be different) API work for you? I am not clear on the second issue you raise though: the thing is that this side information is only needed per frame and before encoding, so I do not see a use case where you keep adding rectangles to this side data. The use case I see is where you accumulate the rectangles and then feed them to the encoding function and forget about them, however, again, if we add an append_rectangles we could easily extend it to the use case you're hinting at. > > Finally, it still seems to me this is largely duplicating > AVVideoEncParams and you could get your functionality by adding your > AVVideoHintType there. > I disagree on this last point. My first idea to avoid duplicating or adding unnecessary code was indeed to extend AVVideoEncParams. However, (please correct me if I am wrong,) to my undestanding the AVVideoEncParams are currently only used at the *decoder* side to convey the encoding parameters (extracted from the bitstream) used by the encoder to produce a stream while here we want to work the other way around: at the encoder's side to provide hints on how to encode a frame but won't affect the bitstream (aside from inducing faster P_SKIPs generation.) and won't be known at the decoder's side. So it seems to me it's a different semantics for which it's better to have an appropriate side information. Best, Elias > -- > Anton Khirnov > _______________________________________________ > 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". NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
Quoting Carotti, Elias (2023-06-22 19:23:05) > On Thu, 2023-06-22 at 10:44 +0200, Anton Khirnov wrote: > > CAUTION: This email originated from outside of the organization. Do > > not click links or open attachments unless you can confirm the sender > > and know the content is safe. > > > > > > > > Quoting Carotti, Elias (2023-06-21 17:53:09) > > > + > > > + /** > > > + * Provide macro block encoder-specific hinting information > > > for the encoder > > > + * processing. It can be used to pass information about which > > > macroblock > > > + * can be skipped because it hasn't changed from the > > > corresponding one in > > > + * the previous frame. This is useful for applications which > > > know in > > > + * advance this information to speed up real-time encoding. > > > Currently only > > > + * used by libx264. > > > > I'd avoid any such claims here, because this comment will certainly > > not > > be kept up to date. > > > Agreed. It was more a statement than a claim, since I only implemented > that :-) > > > > > > +/** > > > + * Allocate memory for a vector of AVVideoRect in the given > > > AVFrame > > > + * {@code frame} as AVFrameSideData of type > > > AV_FRAME_DATA_VIDEO_HINT_INFO. > > > + * The side data contains a list of rectangles for the portions of > > > the frame > > > + * which changed from the last encoded one (and the remainder are > > > assumed to be > > > + * changed), or, alternately (depending on the type parameter) the > > > unchanged > > > + * ones (and the remanining ones are those which changed). > > > + * Macroblocks will thus be hinted either to be P_SKIP-ped or go > > > through the > > > + * regular encoding procedure. > > > + */ > > > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > > > + AVVideoRect *rects, > > > + size_t num_rects, > > > + AVVideoHintType type); > > > + > > > +AVVideoHint *av_video_hint_alloc(AVVideoRect *rects, > > > + size_t nb_rects, > > > + AVVideoHintType type, > > > + size_t *out_size); > > > > If AVVideoHint is extended in the future, you will have a weird > > situation where some fields are set by the allocation function, while > > others have to be set manually by the caller. > > > > You're also assuming the caller has an explicit array of AVVideoRect, > > while they may actually want to fill them in through some other > > means. > > I agree on the first issue, and yes, it would be wiser to split the > allocation function from a the setting function. > Would a simple append_rectangles (the name may be different) API work > for you? I don't see why does there need to be a setting function. The caller can directly assign or memcpy the values, it seems just as easy to me. > I am not clear on the second issue you raise though: the thing is that > this side information is only needed per frame and before encoding, so > I do not see a use case where you keep adding rectangles to this side > data. The use case I see is where you accumulate the rectangles and > then feed them to the encoding function and forget about them, however, > again, if we add an append_rectangles we could easily extend it to the > use case you're hinting at. The case I have in mind is not modifying the data at some later point, but rather that the user has the data in some more complex structure. So in order to fill it here, they'd have to explicitly construct an array of AVVideoRect, which is an extra step. > > > > > Finally, it still seems to me this is largely duplicating > > AVVideoEncParams and you could get your functionality by adding your > > AVVideoHintType there. > > > > I disagree on this last point. My first idea to avoid duplicating or > adding unnecessary code was indeed to extend AVVideoEncParams. However, > (please correct me if I am wrong,) to my undestanding the > AVVideoEncParams are currently only used at the *decoder* side to > convey the encoding parameters (extracted from the bitstream) used by > the encoder to produce a stream while here we want to work the other > way around: at the encoder's side to provide hints on how to encode a > frame but won't affect the bitstream (aside from inducing faster > P_SKIPs generation.) and won't be known at the decoder's side. > > So it seems to me it's a different semantics for which it's better to > have an appropriate side information. AVVideoEncParams describes the block-level parameters of an encoded bitstream. Yes, it is currently used to export coded bitstream information from decoders. But there is no restriction or assumption in the API itself that it will be used in this way and it can just as well describe the information you want a coded bitstream to have.
On Sat, 2023-06-24 at 13:01 +0200, Anton Khirnov wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > Quoting Carotti, Elias (2023-06-22 19:23:05) > > On Thu, 2023-06-22 at 10:44 +0200, Anton Khirnov wrote: > > > CAUTION: This email originated from outside of the organization. > > > Do > > > not click links or open attachments unless you can confirm the > > > sender > > > and know the content is safe. > > > > > > > > > > > > Quoting Carotti, Elias (2023-06-21 17:53:09) > > > > + > > > > + /** > > > > + * Provide macro block encoder-specific hinting > > > > information > > > > for the encoder > > > > + * processing. It can be used to pass information about > > > > which > > > > macroblock > > > > + * can be skipped because it hasn't changed from the > > > > corresponding one in > > > > + * the previous frame. This is useful for applications > > > > which > > > > know in > > > > + * advance this information to speed up real-time > > > > encoding. > > > > Currently only > > > > + * used by libx264. > > > > > > I'd avoid any such claims here, because this comment will > > > certainly > > > not > > > be kept up to date. > > > > > > Agreed. It was more a statement than a claim, since I only > > implemented > > that :-) > > > > > > > > > +/** > > > > + * Allocate memory for a vector of AVVideoRect in the given > > > > AVFrame > > > > + * {@code frame} as AVFrameSideData of type > > > > AV_FRAME_DATA_VIDEO_HINT_INFO. > > > > + * The side data contains a list of rectangles for the > > > > portions of > > > > the frame > > > > + * which changed from the last encoded one (and the remainder > > > > are > > > > assumed to be > > > > + * changed), or, alternately (depending on the type parameter) > > > > the > > > > unchanged > > > > + * ones (and the remanining ones are those which changed). > > > > + * Macroblocks will thus be hinted either to be P_SKIP-ped or > > > > go > > > > through the > > > > + * regular encoding procedure. > > > > + */ > > > > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > > > > + AVVideoRect > > > > *rects, > > > > + size_t num_rects, > > > > + AVVideoHintType > > > > type); > > > > + > > > > +AVVideoHint *av_video_hint_alloc(AVVideoRect *rects, > > > > + size_t nb_rects, > > > > + AVVideoHintType type, > > > > + size_t *out_size); > > > > > > If AVVideoHint is extended in the future, you will have a weird > > > situation where some fields are set by the allocation function, > > > while > > > others have to be set manually by the caller. > > > > > > You're also assuming the caller has an explicit array of > > > AVVideoRect, > > > while they may actually want to fill them in through some other > > > means. > > > > I agree on the first issue, and yes, it would be wiser to split the > > allocation function from a the setting function. > > Would a simple append_rectangles (the name may be different) API > > work > > for you? > > I don't see why does there need to be a setting function. The caller > can > directly assign or memcpy the values, it seems just as easy to me. We can do whatever you want. However I am not clear on how that<br> would work. We could have a side data creation api with the standard parameters and another method to allocate memory so that ownership is kept by libavutil returns a pointer to the rectangles (with bounds checking and so on on the caller): av_video_hint_create_side_data(AVFrame *frame, AVVideoHintType type); AVVideoRect* av_video_hint_set_number_of_rectangles( AVVideoHint *video_hint, size_t n_rects, AVVideoHintType changed_flag); (Names can change I just want to convey a possible api). Would that work for you? Or, do you prefer a creation api which already allocates memory and sets the number of rectangles but doesn't copy them and that's responsibility on the caller? What I'd like in this latter case is that (like now) memory would be flat with no need for specific custom deallocators. Something along the lines: AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, size_t n_rects, AVVideoHintType type); AVVideoRect *av_video_hint_get_rects(AVVideoHint *video_hint); Third option: side information creation api and the caller has to alloc/realloc the rectangle buffer and hand out ownership to libavutil, but I guess this is the worst one for various reasons. I do not see any further option. > > > I am not clear on the second issue you raise though: the thing is > > that > > this side information is only needed per frame and before encoding, > > so > > I do not see a use case where you keep adding rectangles to this > > side > > data. The use case I see is where you accumulate the rectangles and > > then feed them to the encoding function and forget about them, > > however, > > again, if we add an append_rectangles we could easily extend it to > > the > > use case you're hinting at. > > The case I have in mind is not modifying the data at some later > point, > but rather that the user has the data in some more complex structure. > So > in order to fill it here, they'd have to explicitly construct an > array > of AVVideoRect, which is an extra step. > > > > > > > > > Finally, it still seems to me this is largely duplicating > > > AVVideoEncParams and you could get your functionality by adding > > > your > > > AVVideoHintType there. > > > > > > > I disagree on this last point. My first idea to avoid duplicating > > or > > adding unnecessary code was indeed to extend AVVideoEncParams. > > However, > > (please correct me if I am wrong,) to my undestanding the > > AVVideoEncParams are currently only used at the *decoder* side to > > convey the encoding parameters (extracted from the bitstream) used > > by > > the encoder to produce a stream while here we want to work the > > other > > way around: at the encoder's side to provide hints on how to encode > > a > > frame but won't affect the bitstream (aside from inducing faster > > P_SKIPs generation.) and won't be known at the decoder's side. > > > > So it seems to me it's a different semantics for which it's better > > to > > have an appropriate side information. > > AVVideoEncParams describes the block-level parameters of an encoded > bitstream. Yes, it is currently used to export coded bitstream > information from decoders. But there is no restriction or assumption > in > the API itself that it will be used in this way and it can just as > well > describe the information you want a coded bitstream to have. > Right, but in this case it's not something which is going to end up into the bitstream, since this is *not* and api to set some bitstream properties but really just provide some information which the encoder can exploit. So it's definitely a different semantics and I do not think it fits well into AVVideoEncParams (frankly I think it's wrong), however if we are clear on this and that's what you really want and that's what we need to do to get the patch in, well I have no issue changing the code. Best Elias > -- > Anton Khirnov > _______________________________________________ > 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". NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
Quoting Carotti, Elias (2023-06-26 11:50:59) > We can do whatever you want. However I am not clear on how that<br> > would work. > > We could have a side data creation api with the standard parameters and > another method to allocate memory so that ownership is kept by > libavutil returns a pointer to the rectangles (with bounds checking and > so on on the caller): > > > > av_video_hint_create_side_data(AVFrame *frame, AVVideoHintType type); > > AVVideoRect* av_video_hint_set_number_of_rectangles( > AVVideoHint *video_hint, > size_t n_rects, > AVVideoHintType changed_flag); > (Names can change I just want to convey a possible api). > > Would that work for you? > > Or, do you prefer a creation api which already allocates memory and > sets the number of rectangles but doesn't copy them and that's > responsibility on the caller? > What I'd like in this latter case is that (like now) memory would be > flat with no need for specific custom deallocators. > Something along the lines: > > > AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > size_t n_rects, > AVVideoHintType type); > > AVVideoRect *av_video_hint_get_rects(AVVideoHint *video_hint); > > > Third option: side information creation api and the caller has to > alloc/realloc the rectangle buffer and hand out ownership to libavutil, > but I guess this is the worst one for various reasons. > > I do not see any further option. What I'm proposing is this: AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, size_t num_rects); AVVideoHint *av_video_hint_alloc(size_t nb_rects, size_t *out_size); The caller filles the type and the rectangles manually. > > AVVideoEncParams describes the block-level parameters of an encoded > > bitstream. Yes, it is currently used to export coded bitstream > > information from decoders. But there is no restriction or assumption > > in > > the API itself that it will be used in this way and it can just as > > well > > describe the information you want a coded bitstream to have. > > > > > Right, but in this case it's not something which is going to end up > into the bitstream, since this is *not* and api to set some bitstream > properties but really just provide some information which the encoder > can exploit. > > So it's definitely a different semantics and I do not think it fits > well into AVVideoEncParams (frankly I think it's wrong), however if we > are clear on this and that's what you really want and that's what we > need to do to get the patch in, well I have no issue changing the code. Ok, I see your point and drop my objection.
On Wed, 2023-06-28 at 14:55 +0200, Anton Khirnov wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > Quoting Carotti, Elias (2023-06-26 11:50:59) > > We can do whatever you want. However I am not clear on how that<br> > > would work. > > > > We could have a side data creation api with the standard parameters > > and > > another method to allocate memory so that ownership is kept by > > libavutil returns a pointer to the rectangles (with bounds checking > > and > > so on on the caller): > > > > > > > > av_video_hint_create_side_data(AVFrame *frame, AVVideoHintType > > type); > > > > AVVideoRect* av_video_hint_set_number_of_rectangles( > > AVVideoHint *video_hint, > > size_t n_rects, > > AVVideoHintType > > changed_flag); > > (Names can change I just want to convey a possible api). > > > > Would that work for you? > > > > Or, do you prefer a creation api which already allocates memory and > > sets the number of rectangles but doesn't copy them and that's > > responsibility on the caller? > > What I'd like in this latter case is that (like now) memory would > > be > > flat with no need for specific custom deallocators. > > Something along the lines: > > > > > > AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > > size_t n_rects, > > AVVideoHintType type); > > > > AVVideoRect *av_video_hint_get_rects(AVVideoHint *video_hint); > > > > > > Third option: side information creation api and the caller has to > > alloc/realloc the rectangle buffer and hand out ownership to > > libavutil, > > but I guess this is the worst one for various reasons. > > > > I do not see any further option. > > What I'm proposing is this: > AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, size_t > num_rects); > AVVideoHint *av_video_hint_alloc(size_t nb_rects, size_t *out_size); > > The caller filles the type and the rectangles manually. > I implemented the changes, I hope it is better now. The libavcodec part remains unchanged and only this part was affected by the change. Best, Elias > > > AVVideoEncParams describes the block-level parameters of an > > > encoded <snip> > > code. > > Ok, I see your point and drop my objection. NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
Sorry to still nag you, but I just noticed that unlike video_enc_params, you do not store AVVideoRect size in AVVideoHint. This means that no new fields can be added to AVVideoRect without an ABI break. This seems suboptimal, since I do see potential use for per-block information.
On Sat, 2023-07-01 at 10:33 +0200, Anton Khirnov wrote: > CAUTION: This email originated from outside of the organization. Do > not click links or open attachments unless you can confirm the sender > and know the content is safe. > > > > Sorry to still nag you, but I just noticed that unlike > video_enc_params, > you do not store AVVideoRect size in AVVideoHint. This means that > no new fields can be added to AVVideoRect without an ABI break. This > seems suboptimal, since I do see potential use for per-block > information. > Hi Anton, I do agree with you. Please find the updated (and rebased) patch attached to this email. Best, Elias P.S. By the way, I would be tempted (I didn't do that here) to declare the block_size field as const size_t block_size; and then when setting it into av_video_hint_alloc(...) cast away the const (it has to be assigned only once, upon allocation of the struct), just to enforce compiler checking against accidental assignments and for better clarity that this is a read-only field, in the general case. P.P.S.: I'm sending the second part of the patch for libavcodec as a separate email. > -- > Anton Khirnov > _______________________________________________ > 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". NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
On Mon, 2023-07-03 at 15:51 +0000, Carotti, Elias wrote: > On Sat, 2023-07-01 at 10:33 +0200, Anton Khirnov wrote: > > CAUTION: This email originated from outside of the organization. Do > > not click links or open attachments unless you can confirm the > > sender > > and know the content is safe. > > > > > > > > Sorry to still nag you, but I just noticed that unlike > > video_enc_params, > > you do not store AVVideoRect size in AVVideoHint. This means that > > no new fields can be added to AVVideoRect without an ABI break. > > This > > seems suboptimal, since I do see potential use for per-block > > information. > > > > Hi Anton, > I do agree with you. > Please find the updated (and rebased) patch attached to this email. > > Best, > Elias > > P.S. By the way, I would be tempted (I didn't do that here) to > declare > the block_size field as > > const size_t block_size; > > and then when setting it into av_video_hint_alloc(...) cast away the > const (it has to be assigned only once, upon allocation of the > struct), > just to enforce compiler checking against accidental assignments and > for better clarity that this is a read-only field, in the general > case. > > P.P.S.: I'm sending the second part of the patch for libavcodec as a > separate email. Hi Anton, Hi Stefano, do you think this solves the issue? Best Elias NICE SRL, viale Monte Grappa 3/5, 20124 Milano, Italia, Registro delle Imprese di Milano Monza Brianza Lodi REA n. 2096882, Capitale Sociale: 10.329,14 EUR i.v., Cod. Fisc. e P.IVA 01133050052, Societa con Socio Unico
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index cfdd422236..55c64daafa 100644 --- a/libavcodec/libx264.c +++ b/libavcodec/libx264.c @@ -30,6 +30,7 @@ #include "libavutil/stereo3d.h" #include "libavutil/time.h" #include "libavutil/intreadwrite.h" +#include "libavutil/mb_info.h" #include "avcodec.h" #include "codec_internal.h" #include "encode.h" @@ -48,6 +49,9 @@ // from x264.h, for quant_offsets, Macroblocks are 16x16 // blocks of pixels (with respect to the luma plane) #define MB_SIZE 16 +#define MB_LSIZE 4 +#define MB_FLOOR(x) ((x) >> (MB_LSIZE)) +#define MB_CEIL(x) MB_FLOOR((x) + (MB_SIZE - 1)) typedef struct X264Opaque { #if FF_API_REORDERED_OPAQUE @@ -123,6 +127,8 @@ typedef struct X264Context { * encounter a frame with ROI side data. */ int roi_warned; + + int mb_info; } X264Context; static void X264_log(void *p, int level, const char *fmt, va_list args) @@ -295,6 +301,7 @@ static void free_picture(x264_picture_t *pic) av_free(pic->extra_sei.payloads[i].payload); av_freep(&pic->extra_sei.payloads); av_freep(&pic->prop.quant_offsets); + av_freep(&pic->prop.mb_info); pic->extra_sei.num_payloads = 0; } @@ -320,6 +327,45 @@ static enum AVPixelFormat csp_to_pixfmt(int csp) return AV_PIX_FMT_NONE; } +static int setup_mb_info(AVCodecContext *ctx, x264_picture_t *pic, + const AVFrame *frame, const uint8_t *data, + size_t size) +{ + int mb_width = (frame->width + MB_SIZE - 1) / MB_SIZE; + int mb_height = (frame->height + MB_SIZE - 1) / MB_SIZE; + const AVMBInfoRect *mbinfo_rects; + size_t mbinfo_count; + uint8_t *mbinfo; + + mbinfo_rects = (const AVMBInfoRect *)data; + mbinfo_count = size / sizeof(AVMBInfoRect); + + mbinfo = av_calloc(mb_width * mb_height, sizeof(*mbinfo)); + if (!mbinfo) + return AVERROR(ENOMEM); + + /* Sets the default as constant, i.e. P_SKIP-able, then selectively resets the flag */ + memset(mbinfo, X264_MBINFO_CONSTANT, sizeof(*mbinfo) * mb_width * mb_height); + + for (int i = 0; i < mbinfo_count; i++) { + int min_y = MB_FLOOR(mbinfo_rects->y); + int max_y = MB_CEIL(mbinfo_rects->y + mbinfo_rects->height); + int min_x = MB_FLOOR(mbinfo_rects->x); + int max_x = MB_CEIL(mbinfo_rects->x + mbinfo_rects->width); + + for (int mb_y = min_y; mb_y < max_y; ++mb_y) { + memset(mbinfo + mb_y * mb_width + min_x, 0, max_x - min_x); + } + + mbinfo_rects++; + } + + pic->prop.mb_info = mbinfo; + pic->prop.mb_info_free = av_free; + + return 0; +} + static int setup_roi(AVCodecContext *ctx, x264_picture_t *pic, int bit_depth, const AVFrame *frame, const uint8_t *data, size_t size) { @@ -404,6 +450,7 @@ static int setup_frame(AVCodecContext *ctx, const AVFrame *frame, int64_t wallclock = 0; int bit_depth, ret; AVFrameSideData *sd; + AVFrameSideData *mbinfo_sd; *ppic = NULL; if (!frame) @@ -499,6 +546,17 @@ FF_ENABLE_DEPRECATION_WARNINGS goto fail; } + mbinfo_sd = av_frame_get_side_data(frame, AV_FRAME_DATA_MB_INFO); + if (mbinfo_sd) { + int ret = setup_mb_info(ctx, pic, frame, mbinfo_sd->data, mbinfo_sd->size); + if (ret < 0) { + /* No need to fail here, this is not fatal. We just proceed with no + * mb_info and log a message */ + + av_log(ctx, AV_LOG_WARNING, "mb_info setup failure\n"); + } + } + if (x4->udu_sei) { for (int j = 0; j < frame->nb_side_data; j++) { AVFrameSideData *side_data = frame->side_data[j]; @@ -1096,6 +1154,9 @@ static av_cold int X264_init(AVCodecContext *avctx) } } + x4->params.analyse.b_mb_info = x4->mb_info; + x4->params.analyse.b_fast_pskip = 1; + // update AVCodecContext with x264 parameters avctx->has_b_frames = x4->params.i_bframe ? x4->params.i_bframe_pyramid ? 2 : 1 : 0; @@ -1305,6 +1366,7 @@ static const AVOption options[] = { { "noise_reduction", "Noise reduction", OFFSET(noise_reduction), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE }, { "udu_sei", "Use user data unregistered SEI if available", OFFSET(udu_sei), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, { "x264-params", "Override the x264 configuration using a :-separated list of key=value parameters", OFFSET(x264_params), AV_OPT_TYPE_DICT, { 0 }, 0, 0, VE }, + { "mb_info", "Set mb_info data through AVSideData, only useful when used from the API", OFFSET(mb_info), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, { NULL }, }; diff --git a/libavutil/Makefile b/libavutil/Makefile index dc9012f9a8..e99f448213 100644 --- a/libavutil/Makefile +++ b/libavutil/Makefile @@ -91,6 +91,7 @@ HEADERS = adler32.h \ tea.h \ tx.h \ film_grain_params.h \ + mb_info.h \ ARCH_HEADERS = bswap.h \ intmath.h \ @@ -196,6 +197,7 @@ OBJS-$(CONFIG_VAAPI) += hwcontext_vaapi.o OBJS-$(CONFIG_VIDEOTOOLBOX) += hwcontext_videotoolbox.o OBJS-$(CONFIG_VDPAU) += hwcontext_vdpau.o OBJS-$(CONFIG_VULKAN) += hwcontext_vulkan.o +OBJS-$(CONFIG_LIBX264) += mb_info.o OBJS-$(!CONFIG_VULKAN) += hwcontext_stub.o @@ -219,6 +221,8 @@ SKIPHEADERS-$(CONFIG_VULKAN) += hwcontext_vulkan.h vulkan.h \ vulkan_functions.h \ vulkan_loader.h +SKIPHEADERS-$(CONFIG_LIBX264) += mb_info.h + TESTPROGS = adler32 \ aes \ aes_ctr \ diff --git a/libavutil/frame.h b/libavutil/frame.h index f85d630c5c..9c0fdcf25d 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -214,6 +214,16 @@ enum AVFrameSideDataType { * Ambient viewing environment metadata, as defined by H.274. */ AV_FRAME_DATA_AMBIENT_VIEWING_ENVIRONMENT, + + /** + * Provide macro block encoder-specific hinting information for the encoder + * processing. It can be used to pass information about which macroblock + * can be skipped because it hasn't changed from the corresponding one in + * the previous frame. This is useful for applications which know in + * advance this information to speed up real-time encoding. Currently only + * used by libx264. + */ + AV_FRAME_DATA_MB_INFO, }; enum AVActiveFormatDescription { diff --git a/libavutil/mb_info.c b/libavutil/mb_info.c new file mode 100644 index 0000000000..1e4a57b8d4 --- /dev/null +++ b/libavutil/mb_info.c @@ -0,0 +1,51 @@ +/* + * Copyright 2023 Elias Carotti <eliascrt at amazon dot com> + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <string.h> + +#include "avstring.h" +#include "frame.h" +#include "macros.h" +#include "mem.h" +#include "mb_info.h" + + +AVMBInfoRect *av_mb_info_create_side_data(AVFrame *frame, + AVMBInfoRect *rects, + size_t num_rects) +{ + AVFrameSideData *side_data; + AVMBInfoRect *par; + + side_data = av_frame_new_side_data(frame, + AV_FRAME_DATA_MB_INFO, + num_rects * sizeof(AVMBInfoRect)); + + if (!side_data) + return NULL; + + par = (AVMBInfoRect *)side_data->data; + + /* Just copies the rects over the newly allocated buffer */ + memcpy(par, rects, sizeof(AVMBInfoRect) * num_rects); + + return par; +} + diff --git a/libavutil/mb_info.h b/libavutil/mb_info.h new file mode 100644 index 0000000000..918cf167aa --- /dev/null +++ b/libavutil/mb_info.h @@ -0,0 +1,46 @@ +/** + * Copyright 2023 Elias Carotti <eliascrt at amazon dot com> + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#ifndef AVUTIL_MB_INFO_H +#define AVUTIL_MB_INFO_H + +#include <stddef.h> +#include <stdint.h> +#include "libavutil/avassert.h" +#include "libavutil/frame.h" + +typedef struct _AVMBInfoRect { + uint32_t x, y; + uint32_t width, height; +} AVMBInfoRect; + +/** + * Allocate memory for a vector of AVMBInfoRect in the given AVFrame + * {@code frame} as AVFrameSideData of type AV_FRAME_DATA_MB_INFO. + * The side data contains a list of rectangles for the portions of the frame + * which changed from the last encoded one. The rest will be hinted to be + * P_SKIP-ped. Portions of the rects which are not on macroblock boundaries + * are not handled as P_SKIPS. + */ +AVMBInfoRect *av_mb_info_create_side_data(AVFrame *frame, + AVMBInfoRect *rects, + size_t num_rects); + +#endif /* AVUTIL_MB_INFO_H */