[FFmpeg-devel] [PATCH] avfilter: slice processing for geq

Michael Niedermayer michael at niedermayer.cc
Thu Nov 30 02:51:36 EET 2017


On Wed, Nov 29, 2017 at 11:28:40AM +0000, Marc-Antoine ARNAUD wrote:
> Le mer. 22 nov. 2017 à 17:54, Michael Niedermayer <michael at 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 <michael at 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 <onemda at gmail.com> a
> > écrit :
> > > > >
> > > > > > On 11/8/17, Marc-Antoine ARNAUD <arnaud.marcantoine at 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 at 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.marcantoine at 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 at 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.marcantoine at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171130/672df513/attachment.sig>


More information about the ffmpeg-devel mailing list