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