On Thu, Nov 30, 2017 at 01:35:52PM +0000, Marc-Antoine ARNAUD wrote: > Le jeu. 30 nov. 2017 à 01:51, Michael Niedermayer <mich...@niedermayer.cc> > a écrit : > > > On Wed, Nov 29, 2017 at 11:28:40AM +0000, Marc-Antoine ARNAUD wrote: > > > Le mer. 22 nov. 2017 à 17:54, Michael Niedermayer <mich...@niedermayer.cc > > > > > > a écrit : > > > > > > > On Wed, Nov 22, 2017 at 10:24:30AM +0000, Marc-Antoine ARNAUD wrote: > > > > > New patch version which fixe the last remark. > > > > > > > > > > > > > > > Le ven. 10 nov. 2017 à 00:47, Michael Niedermayer > > <mich...@niedermayer.cc > > > > > > > > > > a écrit : > > > > > > > > > > > On Thu, Nov 09, 2017 at 10:22:23AM +0000, Marc-Antoine ARNAUD > > wrote: > > > > > > > Please find the merged patch in attachement. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > Le mer. 8 nov. 2017 à 17:12, Paul B Mahol <one...@gmail.com> a > > > > écrit : > > > > > > > > > > > > > > > On 11/8/17, Marc-Antoine ARNAUD <arnaud.marcanto...@gmail.com> > > > > wrote: > > > > > > > > > This patch will fix the stride issue. > > > > > > > > > Is it valid for you ? > > > > > > > > > > > > > > > > > > Does it required to merge these 2 patches ? (and remove > > base64 > > > > > > encoding > > > > > > > > on > > > > > > > > > the first one) > > > > > > > > > > > > > > > > Please merge those two patches, base64 encoding should not be > > > > needed > > > > > > > > (it helps to faster review patches if they are not encoded). > > > > > > > > _______________________________________________ > > > > > > > > ffmpeg-devel mailing list > > > > > > > > ffmpeg-devel@ffmpeg.org > > > > > > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > > > > > > > > > > > > > > > > vf_geq.c | 124 > > > > > > +++++++++++++++++++++++++++++++++++++++++++++------------------ > > > > > > > 1 file changed, 90 insertions(+), 34 deletions(-) > > > > > > > b41a90fffb5ddef553661007a38659c602f7ce56 > > > > > > 0001-avfilter-slice-processing-for-geq.patch > > > > > > > From ac2a6322fa96835e02a24c31f014fb360e26561f Mon Sep 17 00:00:00 > > > > 2001 > > > > > > > From: Marc-Antoine Arnaud <arnaud.marcanto...@gmail.com> > > > > > > > Date: Thu, 9 Nov 2017 11:19:43 +0100 > > > > > > > Subject: [PATCH] avfilter: slice processing for geq > > > > > > > Content-Type: text/x-patch; charset="utf-8" > > > > > > > > > > > > crashes: > > > > > > ./ffmpeg_g -f lavfi -i > > > > > > > > > > > > 'nullsrc=s=200x200,format=yuv444p16,geq=X*Y/10:sin(X/10)*255:cos(Y/10)*255' > > > > > > -vframes 5 -y blah.avi > > > > > > > > > > > > ==24616== Thread 7: > > > > > > ==24616== Invalid write of size 2 > > > > > > ==24616== at 0x4F3AAF: slice_geq_filter (vf_geq.c:289) > > > > > > ==24616== by 0x48E4C9: worker_func (pthread.c:50) > > > > > > ==24616== by 0x11DB932: run_jobs (slicethread.c:61) > > > > > > ==24616== by 0x11DBA04: thread_worker (slicethread.c:85) > > > > > > ==24616== by 0xC45D183: start_thread (pthread_create.c:312) > > > > > > ==24616== by 0xC770FFC: clone (clone.S:111) > > > > > > ==24616== Address 0x1177143e is 93,214 bytes inside a block of > > size > > > > > > 93,215 alloc'd > > > > > > ==24616== at 0x4C2A6C5: memalign (vg_replace_malloc.c:727) > > > > > > ==24616== by 0x4C2A760: posix_memalign (vg_replace_malloc.c:876) > > > > > > ==24616== by 0x11B0C43: av_malloc (mem.c:87) > > > > > > ==24616== by 0x11987CC: av_buffer_alloc (buffer.c:72) > > > > > > ==24616== by 0x1198831: av_buffer_allocz (buffer.c:85) > > > > > > ==24616== by 0x1198F29: pool_alloc_buffer (buffer.c:312) > > > > > > ==24616== by 0x1199057: av_buffer_pool_get (buffer.c:349) > > > > > > ==24616== by 0x489D6D: ff_frame_pool_get (framepool.c:222) > > > > > > ==24616== by 0x58F6EB: ff_default_get_video_buffer (video.c:89) > > > > > > ==24616== by 0x58F768: ff_get_video_buffer (video.c:102) > > > > > > ==24616== by 0x4F3BF3: geq_filter_frame (vf_geq.c:312) > > > > > > ==24616== by 0x472FD0: ff_filter_frame_framed (avfilter.c:1104) > > > > > > ==24616== by 0x473800: ff_filter_frame_to_filter > > (avfilter.c:1252) > > > > > > ==24616== by 0x4739F8: ff_filter_activate_default > > (avfilter.c:1301) > > > > > > ==24616== by 0x473C12: ff_filter_activate (avfilter.c:1462) > > > > > > ==24616== by 0x478A4F: ff_filter_graph_run_once > > > > (avfiltergraph.c:1456) > > > > > > ==24616== by 0x478C72: get_frame_internal (buffersink.c:110) > > > > > > ==24616== by 0x478CCF: av_buffersink_get_frame_flags > > > > (buffersink.c:121) > > > > > > ==24616== by 0x441808: lavfi_read_packet (lavfi.c:410) > > > > > > ==24616== by 0x7AC315: ff_read_packet (utils.c:822) > > > > > > ==24616== > > > > > > --24616-- VALGRIND INTERNAL ERROR: Valgrind received a signal 11 > > > > (SIGSEGV) > > > > > > - exiting > > > > > > --24616-- si_code=80; Faulting address: 0x0; sp: 0x40a075db0 > > > > > > > > > > > > [...] > > > > > > > > > > > > -- > > > > > > Michael GnuPG fingerprint: > > 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > > > > > > > > > While the State exists there can be no freedom; when there is > > freedom > > > > there > > > > > > will be no State. -- Vladimir Lenin > > > > > > _______________________________________________ > > > > > > ffmpeg-devel mailing list > > > > > > ffmpeg-devel@ffmpeg.org > > > > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > > > > > > > > > > vf_geq.c | 130 > > > > +++++++++++++++++++++++++++++++++++++++++++-------------------- > > > > > 1 file changed, 90 insertions(+), 40 deletions(-) > > > > > abe75c0a0cf89605006905c0c58c0600d26fadb6 > > > > 0001-avfilter-slice-processing-for-geq.patch > > > > > From 7ac2a8c41aaf69ec6cacf7460fa170fd4ca52d8f Mon Sep 17 00:00:00 > > 2001 > > > > > From: Marc-Antoine Arnaud <arnaud.marcanto...@gmail.com> > > > > > Date: Wed, 22 Nov 2017 11:21:35 +0100 > > > > > Subject: [PATCH 1/1] avfilter: slice processing for geq > > > > > Content-Type: text/x-patch; charset="utf-8" > > > > > > > > > > --- > > > > > libavfilter/vf_geq.c | 130 > > > > +++++++++++++++++++++++++++++++++++---------------- > > > > > 1 file changed, 90 insertions(+), 40 deletions(-) > > > > > > > > > > diff --git a/libavfilter/vf_geq.c b/libavfilter/vf_geq.c > > > > > index 36dbd421ce..09bc3d546e 100644 > > > > > --- a/libavfilter/vf_geq.c > > > > > +++ b/libavfilter/vf_geq.c > > > > > @@ -33,15 +33,21 @@ > > > > > #include "libavutil/pixdesc.h" > > > > > #include "internal.h" > > > > > > > > > > +static const char *const var_names[] = { "X", "Y", "W", "H", > > > > "N", "SW", "SH", "T", NULL }; > > > > > +enum { VAR_X, VAR_Y, VAR_W, VAR_H, > > > > VAR_N, VAR_SW, VAR_SH, VAR_T, VAR_VARS_NB }; > > > > > + > > > > > > > > moving this up seem unneeded > > > > > > > > > > > > > typedef struct GEQContext { > > > > > const AVClass *class; > > > > > AVExpr *e[4]; ///< expressions for each plane > > > > > char *expr_str[4+3]; ///< expression strings for each > > plane > > > > > AVFrame *picref; ///< current input buffer > > > > > + uint8_t *dst; ///< reference pointer to the 8bits > > > > output > > > > > + uint16_t *dst16; ///< reference pointer to the 16bits > > > > output > > > > > + double values[VAR_VARS_NB]; ///< expression values > > > > > int hsub, vsub; ///< chroma subsampling > > > > > > > > > + int depth; ///< bit depth of planes > > > > > int planes; ///< number of planes > > > > > int is_rgb; > > > > > - int bps; > > > > > } GEQContext; > > > > > > > > > > enum { Y = 0, U, V, A, G, B, R }; > > > > > @@ -88,7 +94,7 @@ static inline double getpix(void *priv, double x, > > > > double y, int plane) > > > > > x -= xi; > > > > > y -= yi; > > > > > > > > > > - if (geq->bps > 8) { > > > > > + if (geq->depth > 8) { > > > > > const uint16_t *src16 = (const uint16_t*)src; > > > > > linesize /= 2; > > > > > > > > renaming fields should not be in the same patch that does functional > > > > changes. That way changes are easier to read and understand > > > > > > > > > > > > [...] > > > > > @@ -252,34 +311,25 @@ static int geq_filter_frame(AVFilterLink > > *inlink, > > > > AVFrame *in) > > > > > av_frame_copy_props(out, in); > > > > > > > > > > for (plane = 0; plane < geq->planes && out->data[plane]; > > plane++) { > > > > > - int x, y; > > > > > - uint8_t *dst = out->data[plane]; > > > > > - uint16_t *dst16 = (uint16_t*)out->data[plane]; > > > > > + const int width = (plane == 1 || plane == 2) ? > > > > AV_CEIL_RSHIFT(inlink->w, geq->hsub) : inlink->w; > > > > > + const int height = (plane == 1 || plane == 2) ? > > > > AV_CEIL_RSHIFT(inlink->h, geq->vsub) : inlink->h; > > > > > + ThreadData td; > > > > > + > > > > > + geq->dst = out->data[plane]; > > > > > + geq->dst16 = (uint16_t*)out->data[plane]; > > > > > const int linesize = out->linesize[plane]; > > > > > > > > please move the new code after the existing decarations not between > > them > > > > > > > > [...] > > > > > > > > Thanks > > > > > > > > -- > > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > > > > > The worst form of inequality is to try to make unequal things equal. > > > > -- Aristotle > > > > _______________________________________________ > > > > ffmpeg-devel mailing list > > > > ffmpeg-devel@ffmpeg.org > > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > > > > Hello, > > > > > > Thanks for the review. > > > I apply maximum of remarks on this new version of the patch. > > > > > > I just don't take into account the comment about moving var_names and the > > > enum. The struct GeqContext now store an array of expression values and > > > require the VAR_VARS_NB variable to know the array size. Tell me if you > > > think about something better. > > > > you can move the enum up (together with any other "cosmetic" changes) > > in a seperate patch. That way the actual functional changes are not > > obscured by such neccessary moves > > > > Thanks > > > > [...] > > > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > The worst form of inequality is to try to make unequal things equal. > > -- Aristotle > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > Agreed. > Is it better like that ? > > Thanks > Marc-Antoine
> vf_geq.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> c8bb1e8090dc022e8196e1c2466cf9fe058bbb6d
> 0001-avfilter-reorder-variable-definition-in-geq.patch
> From 66093e216580b5edf9b95b23d0055d759a9e2e34 Mon Sep 17 00:00:00 2001
> From: Marc-Antoine Arnaud <arnaud.marcanto...@gmail.com>
> Date: Thu, 30 Nov 2017 10:31:38 +0100
> Subject: [PATCH 1/2] avfilter: reorder variable definition in geq
> Content-Type: text/x-patch; charset="utf-8"
>
> ---
> libavfilter/vf_geq.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libavfilter/vf_geq.c b/libavfilter/vf_geq.c
> index 36dbd421ce..0bd81fd586 100644
> --- a/libavfilter/vf_geq.c
> +++ b/libavfilter/vf_geq.c
> @@ -33,6 +33,9 @@
> #include "libavutil/pixdesc.h"
> #include "internal.h"
>
> +static const char *const var_names[] = { "X", "Y", "W", "H", "N",
> "SW", "SH", "T", NULL };
> +enum { VAR_X, VAR_Y, VAR_W, VAR_H, VAR_N,
> VAR_SW, VAR_SH, VAR_T, VAR_VARS_NB };
> +
> typedef struct GEQContext {
> const AVClass *class;
> AVExpr *e[4]; ///< expressions for each plane
> @@ -107,9 +110,6 @@ static double cb(void *priv, double x, double y) {
> return getpix(priv, x, y, 1)
> static double cr(void *priv, double x, double y) { return getpix(priv, x,
> y, 2); }
> static double alpha(void *priv, double x, double y) { return getpix(priv, x,
> y, 3); }
>
> -static const char *const var_names[] = { "X", "Y", "W", "H", "N",
> "SW", "SH", "T", NULL };
> -enum { VAR_X, VAR_Y, VAR_W, VAR_H, VAR_N,
> VAR_SW, VAR_SH, VAR_T, VAR_VARS_NB };
> -
> static av_cold int geq_init(AVFilterContext *ctx)
> {
> GEQContext *geq = ctx->priv;
> --
> 2.15.0
>
> vf_geq.c | 114
> +++++++++++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 82 insertions(+), 32 deletions(-)
> da656a474c2d60b0045e2fc17c2129d2896cf166
> 0002-avfilter-slice-processing-for-geq.patch
> From 7d8df272f959af7497d0f91775c2a628ba4b256e Mon Sep 17 00:00:00 2001
> From: Marc-Antoine Arnaud <arnaud.marcanto...@gmail.com>
> Date: Thu, 30 Nov 2017 14:34:11 +0100
> Subject: [PATCH 2/2] avfilter: slice processing for geq
> Content-Type: text/x-patch; charset="utf-8"
>
> ---
> libavfilter/vf_geq.c | 114
> ++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 82 insertions(+), 32 deletions(-)
>
> diff --git a/libavfilter/vf_geq.c b/libavfilter/vf_geq.c
> index 0bd81fd586..08ee8b07a6 100644
> --- a/libavfilter/vf_geq.c
> +++ b/libavfilter/vf_geq.c
> @@ -41,6 +41,9 @@ typedef struct GEQContext {
> AVExpr *e[4]; ///< expressions for each plane
> char *expr_str[4+3]; ///< expression strings for each plane
> AVFrame *picref; ///< current input buffer
> + uint8_t *dst; ///< reference pointer to the 8bits output
> + uint16_t *dst16; ///< reference pointer to the 16bits output
> + double values[VAR_VARS_NB]; ///< expression values
> int hsub, vsub; ///< chroma subsampling
> int planes; ///< number of planes
> int is_rgb;
> @@ -226,8 +229,63 @@ static int geq_config_props(AVFilterLink *inlink)
>
> geq->hsub = desc->log2_chroma_w;
> geq->vsub = desc->log2_chroma_h;
> + geq->bps = desc->comp[0].depth;
> geq->planes = desc->nb_components;
> - geq->bps = desc->comp[0].depth;
> +
unneeded
> + return 0;
> +}
> +
> +typedef struct ThreadData {
> + int height;
> + int width;
> + int plane;
> + int linesize;
> +} ThreadData;
> +
> +static int slice_geq_filter(AVFilterContext *ctx, void *arg, int jobnr, int
> nb_jobs)
> +{
> + GEQContext *geq = ctx->priv;
> + ThreadData *td = arg;
> + const int height = td->height;
> + const int width = td->width;
> + const int plane = td->plane;
> + const int linesize = td->linesize;
> + const int slice_start = (height * jobnr) / nb_jobs;
> + const int slice_end = (height * (jobnr+1)) / nb_jobs;
> + int x, y;
> + uint8_t *ptr;
> + uint16_t *ptr16;
> +
> + double values[VAR_VARS_NB];
> + values[VAR_W] = geq->values[VAR_W];
> + values[VAR_H] = geq->values[VAR_H];
> + values[VAR_N] = geq->values[VAR_N];
> + values[VAR_SW] = geq->values[VAR_SW];
> + values[VAR_SH] = geq->values[VAR_SH];
> + values[VAR_T] = geq->values[VAR_T];
> +
> + if (geq->bps == 8) {
> + for (y = slice_start; y < slice_end; y++) {
> + ptr = geq->dst + linesize * y;
> + values[VAR_Y] = y;
> +
> + for (x = 0; x < width; x++) {
> + values[VAR_X] = x;
> + ptr[x] = av_expr_eval(geq->e[plane], values, geq);
> + }
> + ptr += linesize;
> + }
> + }
> + else {
> + for (y = slice_start; y < slice_end; y++) {
> + ptr16 = geq->dst16 + (linesize/2) * y;
> + values[VAR_Y] = y;
> + for (x = 0; x < width; x++) {
> + values[VAR_X] = x;
> + ptr16[x] = av_expr_eval(geq->e[plane], values, geq);
> + }
> + }
> + }
>
> return 0;
> }
> @@ -235,13 +293,14 @@ static int geq_config_props(AVFilterLink *inlink)
> static int geq_filter_frame(AVFilterLink *inlink, AVFrame *in)
> {
> int plane;
> - GEQContext *geq = inlink->dst->priv;
> + AVFilterContext *ctx = inlink->dst;
> + const int nb_threads = ff_filter_get_nb_threads(ctx);
> + GEQContext *geq = ctx->priv;
> AVFilterLink *outlink = inlink->dst->outputs[0];
> AVFrame *out;
> - double values[VAR_VARS_NB] = {
> - [VAR_N] = inlink->frame_count_out,
> - [VAR_T] = in->pts == AV_NOPTS_VALUE ? NAN : in->pts *
> av_q2d(inlink->time_base),
> - };
> +
> + geq->values[VAR_N] = inlink->frame_count_out,
> + geq->values[VAR_T] = in->pts == AV_NOPTS_VALUE ? NAN : in->pts *
> av_q2d(inlink->time_base),
>
> geq->picref = in;
> out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
> @@ -252,34 +311,25 @@ static int geq_filter_frame(AVFilterLink *inlink,
> AVFrame *in)
> av_frame_copy_props(out, in);
>
> for (plane = 0; plane < geq->planes && out->data[plane]; plane++) {
> - int x, y;
> - uint8_t *dst = out->data[plane];
> - uint16_t *dst16 = (uint16_t*)out->data[plane];
> + const int width = (plane == 1 || plane == 2) ?
> AV_CEIL_RSHIFT(inlink->w, geq->hsub) : inlink->w;
> + const int height = (plane == 1 || plane == 2) ?
> AV_CEIL_RSHIFT(inlink->h, geq->vsub) : inlink->h;
> const int linesize = out->linesize[plane];
> - const int w = (plane == 1 || plane == 2) ? AV_CEIL_RSHIFT(inlink->w,
> geq->hsub) : inlink->w;
> - const int h = (plane == 1 || plane == 2) ? AV_CEIL_RSHIFT(inlink->h,
> geq->vsub) : inlink->h;
this should be in the cosmetic patch
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
