From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id B93E844D79 for ; Sun, 23 Jul 2023 23:27:40 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id D072068C697; Mon, 24 Jul 2023 02:27:37 +0300 (EEST) Received: from mail-ej1-f54.google.com (mail-ej1-f54.google.com [209.85.218.54]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id B3F6D68C364 for ; Mon, 24 Jul 2023 02:27:31 +0300 (EEST) Received: by mail-ej1-f54.google.com with SMTP id a640c23a62f3a-991da766865so654799166b.0 for ; Sun, 23 Jul 2023 16:27:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1690154850; x=1690759650; h=user-agent:in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:to:from:date:from:to:cc:subject :date:message-id:reply-to; bh=KGGI7THmSQ6QuihC9grzAhGfzPGvtF1l85h9OVwYmXQ=; b=Voembqb3MrhPWMKI/BGDZQqe1Da6lTFyOlthYlqH6U5J/km2Vx/6hiAcVqvfCEWNe7 jiwsHuwAQY5KbmBdmOLaemFcVwLRYEd2p1jlqPlqcsMif5d9bAA0nS4K3VzE7kMUjuHJ NgrGBW6gZN3SJ8jwbWOPb0BvidKWQAShNytEjWjOMrjGAueM1K6bEiYpDGbfNjhrcZ7w c5lCFYUURq+bdpIgxexyi7HMopu9Y8nK95J7RkD4T3bsPjBB5e3kqJYJxJ+4k1pj2oOg sPgaJYdnmwW1cZthEg1OcxajO5Yh4gtkhuDBlbhtoi3KnUYSEmOeZFl1sjUM+w08Id6M gtRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690154850; x=1690759650; h=user-agent:in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=KGGI7THmSQ6QuihC9grzAhGfzPGvtF1l85h9OVwYmXQ=; b=AK5f6u0zM2jsi36Ko0rIQT3sjv6bZ9Plht9chvD1f+kUki0JqTk0pfAbUbTjmQkqRw ZFtQBo2MzqfQlbaE0KMTa7mxcQyOJtu9+QNJ5P1tITdGkL1wXyU1K12/HJoULUl2nD+8 etaniobf5oxGinpckvP+iXotYwb0OzUNSfqWXpJMB4fjU5yRJXVQ69KKHmvPAh8MwvpE NRtjd5A+Bm1598OJq3FWj5Gtt9h/sh/Va1pAa871pUp01DX299sW8dfUUZDjtWmFlODS YIgzOm4zMiGEH5tVA9b2kuVzHaZFuwDDjU1JMtXQhVLoZKR6kbGoqBJThdOzHB3dJd+g WGDA== X-Gm-Message-State: ABy/qLbmds/zKDd/DIxZzZ02IMomtPktpxhgrxjWJo0kCyFflNDGoL6/ FGO3mcrg2C3ghvX1VVSgDQb2rUZJjKw= X-Google-Smtp-Source: APBJJlEVCYdosVR2ORfr7HiFAhRmL/sbOrY1rE874TMoWiMgHlPJj5EE07sYJWCXrEtNeEg9MGLF6g== X-Received: by 2002:a17:906:53d0:b0:994:4e16:d430 with SMTP id p16-20020a17090653d000b009944e16d430mr9639994ejo.20.1690154850058; Sun, 23 Jul 2023 16:27:30 -0700 (PDT) Received: from mariano ([91.254.11.233]) by smtp.gmail.com with ESMTPSA id c26-20020a170906925a00b00982be08a9besm5896294ejx.172.2023.07.23.16.27.28 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 23 Jul 2023 16:27:29 -0700 (PDT) Received: by mariano (Postfix, from userid 1000) id AEA18BFB73; Mon, 24 Jul 2023 01:27:27 +0200 (CEST) Date: Mon, 24 Jul 2023 01:27:27 +0200 From: Stefano Sabatini To: FFmpeg development discussions and patches Message-ID: <20230723232727.GB10927@mariano> Mail-Followup-To: FFmpeg development discussions and patches References: <168795695865.21886.2879227621245771333@lain.khirnov.net> <168820043273.21886.17648463695363628461@lain.khirnov.net> <20230709110529.29490-1-anton@khirnov.net> <2ef05e377ecdcb086d2ec00a0c8d810ff962ca77.camel@amazon.it> <20230717221940.GC10400@mariano> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230717221940.GC10400@mariano> User-Agent: Mutt/1.13.2 (2019-12-18) Subject: Re: [FFmpeg-devel] [PATCH] lavu: add AVVideoHint API X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On date Tuesday 2023-07-18 00:19:40 +0200, Stefano Sabatini wrote: > On date Monday 2023-07-10 12:51:25 +0000, Carotti, Elias wrote: > > On Mon, 2023-07-10 at 08:13 +0000, Carotti, Elias wrote: > > > > > > > > AVVideoHint is a bad name for something like this. > > > Could you borrow some wording from graphics and call it > > > AVVideoDamagedHint or maybe AVVideoChangedAreaHint or a combination > > > of both? > > > I'd prefer the former, damage is standard language in graphics > > > circles about what has changed since the last frame. > > > > > > Hi, > > > I have no strong objections on this. Personally I also like the > > > AVVideoDamagedHint name best, my only concern is that it is strictly > > > related to the current use/implementation > > > (it's true right now that's the only kind of hint) while it may turn > > > out to be a bad naming decision should other forms of hinting for the > > > encoder be added in the future. > > > That said, I am fine with the change too. > > AVVideoDamagedHint would be too specific (what if we want to add > "hinting" for different things?). > > > > Elias > > > > > > > I added a type to the AVVideoRect struct. This should solve the naming > > issue above while preserving the possibility to extend this to > > different hinting types. > > These are the only changes to Anton's version. > > 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 > > > > > > > From 8ef4f97410a6b78df048b71d9921a763da6255b3 Mon Sep 17 00:00:00 2001 > > From: Elias Carotti > > Date: Mon, 10 Jul 2023 14:34:53 +0200 > > Subject: [PATCH] Add side data type to provide hint to the video encoders > > about unchanged portions of each frame. > > > > Signed-off-by: Anton Khirnov > > --- > > doc/APIchanges | 3 ++ > > libavutil/Makefile | 2 + > > libavutil/frame.h | 10 ++++ > > libavutil/version.h | 2 +- > > libavutil/video_hint.c | 82 +++++++++++++++++++++++++++++ > > libavutil/video_hint.h | 117 +++++++++++++++++++++++++++++++++++++++++ > > 6 files changed, 215 insertions(+), 1 deletion(-) > > create mode 100644 libavutil/video_hint.c > > create mode 100644 libavutil/video_hint.h > > > > diff --git a/doc/APIchanges b/doc/APIchanges > > index 27f835cfce..0cda51fdee 100644 > > --- a/doc/APIchanges > > +++ b/doc/APIchanges > > @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09 > > > > API changes, most recent first: > > > > +2023-07-xx - xxxxxxxxxx - lavu 58.15.100 - video_hint.h > > + Add AVVideoHint API. > > + > > 2023-07-05 - xxxxxxxxxx - lavu 58.14.100 - random_seed.h > > Add av_random_bytes() > > > > diff --git a/libavutil/Makefile b/libavutil/Makefile > > index bd9c6f9e32..7828c94dc5 100644 > > --- a/libavutil/Makefile > > +++ b/libavutil/Makefile > > @@ -91,6 +91,7 @@ HEADERS = adler32.h \ > > tea.h \ > > tx.h \ > > film_grain_params.h \ > > + video_hint.h > > > > ARCH_HEADERS = bswap.h \ > > intmath.h \ > > @@ -181,6 +182,7 @@ OBJS = adler32.o \ > > uuid.o \ > > version.o \ > > video_enc_params.o \ > > + video_hint.o \ > > film_grain_params.o \ > > > > > > diff --git a/libavutil/frame.h b/libavutil/frame.h > > index a491315f25..c0c1b23db7 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 encoder-specific hinting information about changed/unchanged > > + * portions of a frame. It can be used to pass information about which > > + * macroblocks can be skipped because they didn't change from the > > + * corresponding ones in the previous frame. This could be useful for > > + * applications which know this information in advance to speed up > > + * encoding. > > + */ > > + AV_FRAME_DATA_VIDEO_HINT, > > }; > > > > enum AVActiveFormatDescription { > > diff --git a/libavutil/version.h b/libavutil/version.h > > index 24af520e08..9e798b0e3f 100644 > > --- a/libavutil/version.h > > +++ b/libavutil/version.h > > @@ -79,7 +79,7 @@ > > */ > > > > #define LIBAVUTIL_VERSION_MAJOR 58 > > -#define LIBAVUTIL_VERSION_MINOR 14 > > +#define LIBAVUTIL_VERSION_MINOR 15 > > #define LIBAVUTIL_VERSION_MICRO 100 > > > > #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ > > diff --git a/libavutil/video_hint.c b/libavutil/video_hint.c > > new file mode 100644 > > index 0000000000..5730ed6cfb > > --- /dev/null > > +++ b/libavutil/video_hint.c > > @@ -0,0 +1,82 @@ > > +/* > > + * Copyright 2023 Elias Carotti > > + * > > + * 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 > > + > > +#include "avstring.h" > > +#include "frame.h" > > +#include "macros.h" > > +#include "mem.h" > > +#include "video_hint.h" > > + > > +AVVideoHint *av_video_hint_alloc(size_t nb_rects, > > + size_t *out_size) > > +{ > > + struct TestStruct { > > + AVVideoHint hint; > > + AVVideoRect rect; > > + }; > > + const size_t rect_offset = offsetof(struct TestStruct, rect); > > + size_t size = rect_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->nb_rects = nb_rects; > > + hint->rect_offset = rect_offset; > > + hint->rect_size = sizeof(AVVideoRect); > > + > > + *out_size = size; > > + > > + return hint; > > +} > > + > > +AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, > > + size_t nb_rects) > > +{ > > + AVVideoHint *hint; > > + AVBufferRef *buf; > > + size_t size = 0; > > + > > + hint = av_video_hint_alloc(nb_rects, &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.h b/libavutil/video_hint.h > > new file mode 100644 > > index 0000000000..14bfe20aa6 > > --- /dev/null > > +++ b/libavutil/video_hint.h > > @@ -0,0 +1,117 @@ > > +/** > > + * Copyright 2023 Elias Carotti > > + * > > + * 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_H > > +#define AVUTIL_VIDEO_HINT_H > > + > > +#include > > +#include > > +#include "libavutil/avassert.h" > > +#include "libavutil/frame.h" > > + > > > +/** > > + * The type of the hinting information provided by an AVVideoRect. > > + * Currently only damage information, i.e., changed/unchanged portions of the frame. > > + **/ > > > +typedef enum AVVideoRectType { > > + AV_VIDEO_RECT_DAMAGE, > > AV_VIDEO_RECT_TYPE_DAMAGE for consistency > > > +} AVVideoRectType; > > > + > > +typedef struct AVVideoRect { > > + uint32_t x, y; > > + uint32_t width, height; > > + AVVideoRectType type; > > +} AVVideoRect; > > + > > +typedef enum AVVideoHintType { > > + /* rectangled delimit the constant areas (unchanged), default is changed */ > > rectangles? > > > + AV_VIDEO_HINT_CONSTANT, > > AV_VIDEO_HINT_TYPE_... > > > + > > + /* rectangled delimit the constant areas (changed), default is not changed */ > > rectangles? > > > + AV_VIDEO_HINT_CHANGED, > > +} AVVideoHintType; > > > > +typedef struct AVVideoHint { > > + /** > > + * Number of AVVideoRect present. > > + * > > + * May be 0, in which case no per-rectangle information is present. In this > > + * case the values of rect_offset / rect_size are unspecified and should > > + * not be accessed. > > + */ > > + size_t nb_rects; > > + > > + /** > > + * Offset in bytes from the beginning of this structure at which the array > > + * of AVVideoRect starts. > > + */ > > + size_t rect_offset; > > + > > + /** > > + * Size in bytes of AVVideoRect. > > + */ > > + size_t rect_size; > > + > > > + AVVideoHintType type; > > sorry to nitpick, but if we have this information in the single rect > should not we use that instead? > > Basically I see two options: > 1. generic VideoHint, storing single hint type and rects > > for example we might have type=changed, and the rects storing only the > changed rects > > If we want to extend this then we need to add a new type. The problem > might be if we want to store more than one VideoHint in the side data > (it it possible to store more than one AVVideoHint, each one with a > different type, as side data?). Suppose for example we want to add a > new hint type, e.g. ROI, then we can add a new AVHideoHint with > type=roi - but I don't know if we can have several frame hint side > data (each one with different individual types). > > 2. generic VideoHint, storing rects each one containing the hint type > > for example we might have a list of rects, each one with > type=changed|constant|roi. This would allow one to store different > kind of hints in the same AVVideoHint structure, but at the same time > would enable inconsistent data (for example we might have changed and > unchanged rects in the same list of rects) > > In each case, storing the type in the generic AVVideHint and in each > rect might lead to inconsistent states (e.g. you might have generic > type=damage, and have type=roi and type=changed in the rects), in this > case having the type in both the general structure and in each rects > seems redundant and leads to possible data inconsistencies. > > ... > > I would rather go to solution 1, but I'm not sure if having more than > one hint in the side data (with different subtypes) is possible and > wanted. If this is the case, probably the best solution would be to > store more than one AVVideoHint (each one with an individual type and > list of rects) in the single side data object. Elaborating on this. 1. Ideally we might want to extend the av_frame_get_side_data API to be able to store more than one entry per side data, by adding a unique label to the side data, but this requires probably more effort and extends the scope even further. 2. Alternatively, we might extend the hint API like this: AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, size_t nb_rects); AVVideoHint *av_video_hint_extend_side_data(AVFrame *frame, size_t nb_rects); by marking the continuation on the previous hint (but this would complicate deserialization as you need then to iterate to get all the side data). Or we could make a variadic function like this: AVVideoHint *av_video_hint_create_side_data(AVFrame *frame, size_t nb_rects, ...); Alternatively, we might store in AVVideoHint more than one hint, but this somehow conflict with the general design. ... As a low effort approach, I would keep the initial design of a single hint type per side-data (dropping the possibility of having more than one hint type in the same side data), which should be fixed generically by implementing 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".