Re: [FFmpeg-devel] [PATCH] Added support for MB_INFO

["Carotti, Elias" <eliascrt@amazon.it>]

[-- Attachment #1: Type: text/plain, Size: 2365 bytes --]

Hi,
thanks for pointing out the printf. That's a left over which I removed.

I am not clear on the possible leak you are hinting at. 
The new side information only passes two pointers to libx264, the first
one being a buffer with the flags and a pointer to a deallocator which
may be NULL. 

If the deallocator is not NULL libx264 we're yielding the deallocation
responsibility to libx264, otherwise the ownership of the buffer and,
as such, the responsibility for the deallocation remains with the
caller. 
As such, the only possibility for a leak seems to me due to a
programming error.
Is that what you were asking or am I missing something else?

Please find attached the updated patch.

Elias



On Fri, 2022-06-17 at 11:40 +0200, Timo Rothenpieler 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 17.06.2022 10:41, Carotti, Elias wrote:
> > Hi all,
> > any chance someone could possibly have a look at this patch,
> > please?
> > Thanks in advance
> > 
> > 
> > 
> > On Fri, 2022-06-10 at 10:11 +0000, Carotti, Elias wrote:
> > > Hi,
> > > patch attached to add support for passing down to libx264
> > > information
> > > about which macroblock could be eligible for being coded as
> > > P_SKIP.
> > > 
> > 
> > 
> > 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
> > 
> 
> There's a stray printf in the patch.
> 
> Also, what if a frame with such side data is passed to not-x264?
> Won't
> it leak the memory?
>  From the looks if it, this entirely relies on x264 to free it.
> _______________________________________________
> 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



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Added-support-for-MB_INFO.patch --]
[-- Type: text/x-patch; name="0001-Added-support-for-MB_INFO.patch", Size: 9837 bytes --]

From 8f5653f8c9bc204fec6af341b80a59ef5edfc168 Mon Sep 17 00:00:00 2001
From: Elias Carotti <eliascrt@amazon.it>
Date: Thu, 26 May 2022 21:17:43 +0000
Subject: [PATCH] Added support for MB_INFO

---
 doc/APIchanges       |  4 ++++
 libavcodec/libx264.c | 21 +++++++++++++++++++
 libavutil/Makefile   |  4 ++++
 libavutil/frame.h    | 10 +++++++++
 libavutil/mb_info.c  | 43 +++++++++++++++++++++++++++++++++++++
 libavutil/mb_info.h  | 50 ++++++++++++++++++++++++++++++++++++++++++++
 libavutil/version.h  |  2 +-
 7 files changed, 133 insertions(+), 1 deletion(-)
 create mode 100644 libavutil/mb_info.c
 create mode 100644 libavutil/mb_info.h

diff --git a/doc/APIchanges b/doc/APIchanges
index 20b944933a..49fd551138 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,10 @@ libavutil:     2021-04-27
 
 API changes, most recent first:
 
+2022-06-17 - xxxxxxxxx - lavu 57.28.100 - frame.h
+  Add AV_FRAME_DATA_MB_INFO to AVFrameSideDataType and the supporting AVMBInfo
+  API.
+
 2022-06-12 - xxxxxxxxxx - lavf 59.25.100 - avio.h
   Add avio_vprintf(), similar to avio_printf() but allow to use it
   from within a function taking a variable argument list as input.
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 14177b3016..161da1029e 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -29,6 +29,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"
@@ -115,6 +116,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)
@@ -491,6 +494,20 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame,
         }
     }
 
+    if (frame && x4->mb_info) {
+        AVFrameSideData *mbinfo_sd = av_frame_get_side_data(frame, AV_FRAME_DATA_MB_INFO);
+
+        if (mbinfo_sd) {
+            AVMBInfo *par = (AVMBInfo *)mbinfo_sd->data;
+
+            x4->pic.prop.mb_info = par->mb_info;
+            x4->pic.prop.mb_info_free = par->mb_info_free;
+        } else if (!mbinfo_sd || !mbinfo_sd->data) {
+            av_log(ctx, AV_LOG_WARNING,
+                    "mb_info flag set but no actual MB info was provided\n");
+        }
+    }
+
     do {
         if (x264_encoder_encode(x4->enc, &nal, &nnal, frame? &x4->pic: NULL, &pic_out) < 0)
             return AVERROR_EXTERNAL;
@@ -965,6 +982,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;
@@ -1172,6 +1192,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 29c170214c..6af99f6d0d 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -90,6 +90,7 @@ HEADERS = adler32.h                                                     \
           tea.h                                                         \
           tx.h                                                          \
           film_grain_params.h                                           \
+          mb_info.h							\
 
 ARCH_HEADERS = bswap.h                                                  \
                intmath.h                                                \
@@ -195,6 +196,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 += $(COMPAT_OBJS:%=../compat/%)
 
@@ -216,6 +218,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 33fac2054c..08c84a1b63 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -209,6 +209,16 @@ enum AVFrameSideDataType {
      * volume transform - CUVA 005.1-2021.
      */
     AV_FRAME_DATA_DYNAMIC_HDR_VIVID,
+
+    /**
+     * 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..d27d8dc412
--- /dev/null
+++ b/libavutil/mb_info.c
@@ -0,0 +1,43 @@
+/*
+ * Copyright (c) 2022 Elias Carotti eliascrt _AT_ amazon.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 "mb_info.h"
+
+
+AVMBInfo *av_mb_info_create_side_data(AVFrame *frame, uint8_t* mb_info,
+                                            void (*mb_info_free)(void *))
+{
+    AVFrameSideData *side_data = av_frame_new_side_data(frame,
+                                                        AV_FRAME_DATA_MB_INFO,
+                                                        sizeof(AVMBInfo));
+    AVMBInfo *par = (AVMBInfo *)side_data->data;
+
+    par->mb_info = mb_info;
+    par->mb_info_free = mb_info_free;
+
+    return par;
+}
+
diff --git a/libavutil/mb_info.h b/libavutil/mb_info.h
new file mode 100644
index 0000000000..007f276dc4
--- /dev/null
+++ b/libavutil/mb_info.h
@@ -0,0 +1,50 @@
+/**
+ * Copyright (c) 2022 Elias Carotti eliascrt _AT_ amazon.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_MB_INFO_H
+#define AVUTIL_MB_INFO_H
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include "libavutil/avassert.h"
+#include "libavutil/frame.h"
+
+
+typedef struct AVMBInfo {
+    /**
+     * The actual mb_info data: one uint8_t per macroblock in raster-scan order.
+     * Currently the only flag defined in x264.h is X264_MB_INFO_CONSTANT
+     */
+    uint8_t *mb_info;
+
+    /* An optional pointer (may be NULL) to a de-allocator for the mb_info data */
+    void (*mb_info_free)(void *);
+} AVMBInfo;
+
+/**
+ * Allocate memory for AVMBInfo in the given AVFrame {@code frame}
+ * as AVFrameSideData of type AV_FRAME_DATA_MB_INFO
+ * and initializes the variables.
+ */
+AVMBInfo *av_mb_info_create_side_data(AVFrame *frame, uint8_t* mb_info,
+                                            void (*mb_info_free)(void *));
+
+#endif /* AVUTIL_MB_INFO_H */
diff --git a/libavutil/version.h b/libavutil/version.h
index 2e9e02dda8..87178e9e9a 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  57
-#define LIBAVUTIL_VERSION_MINOR  27
+#define LIBAVUTIL_VERSION_MINOR  28
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
-- 
2.25.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".