Hi Andrea I havnt really followed your gsoc work so in case some of my comments below differs from your mentor(s), please point that out to me.
also maybe your mentors can do a full review (in CC) as it seems
noone else has reviewed your patch.
thx
On Thu, Dec 05, 2024 at 06:59:29PM +0100, AndreaMastroberti wrote:
[...]> diff --git a/libavfilter/version.h b/libavfilter/version.h
> index f84dec4805..0050874108 100644
> --- a/libavfilter/version.h
> +++ b/libavfilter/version.h
> @@ -31,8 +31,8 @@
>
> #include "version_major.h"
>
> -#define LIBAVFILTER_VERSION_MINOR 6
> -#define LIBAVFILTER_VERSION_MICRO 101
> +#define LIBAVFILTER_VERSION_MINOR 7
> +#define LIBAVFILTER_VERSION_MICRO 100
>
>
> #define LIBAVFILTER_VERSION_INT AV_VERSION_INT(LIBAVFILTER_VERSION_MAJOR, \
> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
> index 52b22a6870..53b9af4d26 100644
> --- a/libavfilter/vf_ssim.c
> +++ b/libavfilter/vf_ssim.c
> @@ -44,6 +44,7 @@
> #include "filters.h"
> #include "framesync.h"
> #include "ssim.h"
> +#include <math.h>
>
> typedef struct SSIMContext {
> const AVClass *class;
> @@ -63,8 +64,15 @@ typedef struct SSIMContext {
> int **temp;
> int is_rgb;
> double **score;
> + uint64_t **i1[4], **i2[4], **s1[4], **s2[4], **i12[4];
> + int int_img;
> + int win_size;
> + int stride;
> + PoolMethod pool;
> int (*ssim_plane)(AVFilterContext *ctx, void *arg,
> int jobnr, int nb_jobs);
> + int (*ssim_plane_int)(AVFilterContext *ctx, void *arg,
> + int jobnr, int nb_jobs);
> SSIMDSPContext dsp;
> } SSIMContext;
>
> @@ -74,6 +82,13 @@ typedef struct SSIMContext {
> static const AVOption ssim_options[] = {
> {"stats_file", "Set file where to store per-frame difference
> information", OFFSET(stats_file_str), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0,
> FLAGS },
> {"f", "Set file where to store per-frame difference
> information", OFFSET(stats_file_str), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0,
> FLAGS },
> + { "int_img", "Compute SSIM using integral images", OFFSET(int_img),
> AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
> + { "window", "Window size", OFFSET(win_size), AV_OPT_TYPE_INT, { .i64 = 8
> }, 8, 16, FLAGS },
> + { "stride", "Stride length", OFFSET(stride), AV_OPT_TYPE_INT, { .i64 = 4
> }, 4, 8, FLAGS },
> + { "pool", "Pooling method", OFFSET(pool), AV_OPT_TYPE_INT, {.i64 = AVG},
> 0, 2, FLAGS, "pool" },
> + { "avg", "Average pooling", 0, AV_OPT_TYPE_CONST, {.i64 = AVG}, 0,
> 0, FLAGS, "pool" },
> + { "mink3", "Minkowski norm 3", 0, AV_OPT_TYPE_CONST, {.i64 =
> MINK_3}, 0, 0, FLAGS, "pool" },
> + { "mink4", "Minkowski norm 4", 0, AV_OPT_TYPE_CONST, {.i64 =
> MINK_4}, 0, 0, FLAGS, "pool" },
> { NULL }
> };
>
These can be vertically aligned like:
> + { "avg", "Average pooling", 0, AV_OPT_TYPE_CONST, {.i64 =
> AVG}, 0, 0, FLAGS, "pool" },
> + { "mink3", "Minkowski norm 3", 0, AV_OPT_TYPE_CONST, {.i64 =
> MINK_3}, 0, 0, FLAGS, "pool" },
> + { "mink4", "Minkowski norm 4", 0, AV_OPT_TYPE_CONST, {.i64 =
> MINK_4}, 0, 0, FLAGS, "pool" },
> @@ -175,6 +190,28 @@ static float ssim_end1x(int64_t s1, int64_t s2, int64_t
> ss, int64_t s12, int max
> / ((float)(fs1 * fs1 + fs2 * fs2 + ssim_c1) * (float)(vars +
> ssim_c2));
> }
>
> +static float ssim_end1w(int s1, int s2, int ss, int s12, int win_size)
> +{
> + double ws = (double)win_size * win_size;
> + double ssim_c1 = 0.01 * 0.01 * 255 * 255 * ws;
> + double ssim_c2 = 0.03 * 0.03 * 255 * 255 * ws * (ws - 1);
> +
> + double fs1 = (double)s1;
> + double fs2 = (double)s2;
> + double fss = (double)ss;
> + double fs12 = (double)s12;
> +
> + double vars = fss * ws - fs1 * fs1 - fs2 * fs2;
> + double covar = fs12 * ws - fs1 * fs2;
this can be calculated in int or int64 avoiding rounding issues with floats
> +
> + double num = (2 * fs1 * fs2 + ssim_c1) * (2 * covar + ssim_c2);
if you multiply
0.01 * 0.01 * 255 * 255 * ws
by 50 then its an integer and you could avoid floats (assuming ws is 8 or 16)
before the num / den
not sure how usefull it is to avoid float/double here but as the input
is integers it seemed an idea that may be interresting
this way the numbers could be kept exact until num / den below i think
[...]
> +static int ssim_plane_int(AVFilterContext *ctx, void *arg,
> + int jobnr, int nb_jobs)
> +{
> + SSIMContext *s = ctx->priv;
> + ThreadData *td = arg;
> + double *score = td->score[jobnr];
> + void *temp = td->temp[jobnr];
> + int stride = s->stride, win_size = s->win_size;
> + uint64_t *i1, *i2, *s1, *s2, *i12;
> + int offset;
> +
> + for (int c = 0; c < td->nb_components; c++) {
> + int width = td->planewidth[c];
> + int height = td->planeheight[c];
> + const int slice_start = ((height/stride) * jobnr) / nb_jobs;
> + const int slice_end = ((height/stride) * (jobnr+1)) / nb_jobs;
how does this behave if height is not a multiple of stride ?
does this work as intended in that case ?
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
