From patchwork Fri Jan 6 18:34:03 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gerion Entrup X-Patchwork-Id: 2077 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.89.21 with SMTP id n21csp5951607vsb; Fri, 6 Jan 2017 10:34:15 -0800 (PST) X-Received: by 10.28.104.86 with SMTP id d83mr4171999wmc.21.1483727655313; Fri, 06 Jan 2017 10:34:15 -0800 (PST) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id to4si9951523wjc.191.2017.01.06.10.34.14; Fri, 06 Jan 2017 10:34:15 -0800 (PST) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 973D868A2D4; Fri, 6 Jan 2017 20:34:05 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail.flump.de (unknown [84.200.20.107]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 557D368A2D4 for ; Fri, 6 Jan 2017 20:33:59 +0200 (EET) Received: from gump.localnet (p57931DB6.dip0.t-ipconnect.de [87.147.29.182]) by mail.flump.de (Postfix) with ESMTPSA id 8810018216A for ; Fri, 6 Jan 2017 19:34:02 +0100 (CET) From: Gerion Entrup To: FFmpeg development discussions and patches Date: Fri, 06 Jan 2017 19:34:03 +0100 Message-ID: <3632169.O44Xb1qJOu@gump> In-Reply-To: <20170105012623.GY4749@nb4> References: <4126592.Myufu8ebLk@gump> <2644280.rG1UgZZ4cT@gump> <20170105012623.GY4749@nb4> MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCHv3] add signature filter for MPEG7 video signature X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 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 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" On Donnerstag, 5. Januar 2017 02:26:23 CET Michael Niedermayer wrote: > On Wed, Jan 04, 2017 at 05:05:41PM +0100, Gerion Entrup wrote: > > On Dienstag, 3. Januar 2017 16:58:32 CET Moritz Barsnick wrote: > > > > > The English opposite of "fine" is "coarse", not "course". :) > > > > Oops. > > > > > > You still have a few "courses". (The actual variables, not the types. I > > > don't care *too* much, but might be better for consistency.) > > You're right. Fixed version attached. > > > > > > > From my side - mostly style and docs - it looks fine. Technically, I > > > can't judge too much. You went through a long review cycle already, so > > > I assume it's fine for those previous reviewers. They have the last > > > call anyway. > > > > What about FATE? I'm willing to write a test, but don't know the best way. > > There are official conditions, whether the signature is standard compliant. I've > > written a python script to proof that (see previous emails). Nevertheless the > > checks take relatively long and need 3GB inputvideo, so I assume this is not > > usable for FATE. > > yes, a 3gb reference is not ok for fate > > > > > > One way would be, to take a short input video, take the calculated signature > > and use this as reference, but the standard allow some bitflips and my code > > has some of them in comparison to the reference signatures. > > then the fate test could/should compare and pass if its within what > we expect of our code. (which may be stricter than the reference > allowance) > there are already tests that do similar for comparing PCM/RAW Ok, will try to create a test the next days. > > +#define OFFSET(x) offsetof(SignatureContext, x) > > > +#define FLAGS AV_OPT_FLAG_VIDEO_PARAM > > should contin also AV_OPT_FLAG_FILTERING_PARAM Done. > > +static int export(AVFilterContext *ctx, StreamContext *sc, int input) > > +{ > > + SignatureContext* sic = ctx->priv; > > + char filename[1024]; > > + > > + if (sic->nb_inputs > 1) { > > > + /* error already handled */ > > + av_get_frame_filename(filename, sizeof(filename), sic->filename, input); > > its more robust to use a av_assert*() on the return code if its assumed > to be never failing than just a comment as a comment cannot be > automatcially checked for validity currently. I chose av_assert0 because the check is done only nb_inputs times. The attached patch also fixes the file writing for every frame the one input is longer than the other. Gerion diff --git a/libavfilter/vf_signature.c b/libavfilter/vf_signature.c index 3e62c68..57cb96b 100644 --- a/libavfilter/vf_signature.c +++ b/libavfilter/vf_signature.c @@ -37,7 +37,7 @@ #include "signature_lookup.c" #define OFFSET(x) offsetof(SignatureContext, x) -#define FLAGS AV_OPT_FLAG_VIDEO_PARAM +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM | AV_OPT_FLAG_VIDEO_PARAM #define BLOCK_LCM (int64_t) 476985600 static const AVOption signature_options[] = { @@ -571,7 +571,7 @@ static int export(AVFilterContext *ctx, StreamContext *sc, int input) if (sic->nb_inputs > 1) { /* error already handled */ - av_get_frame_filename(filename, sizeof(filename), sic->filename, input); + av_assert0(av_get_frame_filename(filename, sizeof(filename), sic->filename, input) == 0); } else { strcpy(filename, sic->filename); } @@ -580,7 +580,6 @@ static int export(AVFilterContext *ctx, StreamContext *sc, int input) } else { return binary_export(ctx, sc, filename); } - } static int request_frame(AVFilterLink *outlink) @@ -603,7 +602,7 @@ static int request_frame(AVFilterLink *outlink) return ret; /* export signature at EOF */ - if (ret == AVERROR_EOF) { + if (ret == AVERROR_EOF && !sc->exported) { /* export if wanted */ if (strlen(sic->filename) > 0) { if (export(ctx, sc, i) < 0)