| Message ID | 3632169.O44Xb1qJOu@gump |
|---|---|
| State | Accepted |
| Headers | show |
On Jan 6, 2017, at 1:34 PM, Gerion Entrup <gerion.entrup.ffdev@flump.de> wrote: > > 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. Just bumping this thread. I've been using the patch and find it very helpful and would like to see it in libavfilter. Dave Rice
On 3/20/17, Dave Rice <dave@dericed.com> wrote: > On Jan 6, 2017, at 1:34 PM, Gerion Entrup <gerion.entrup.ffdev@flump.de> > wrote: >> >> 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. > > Just bumping this thread. I've been using the patch and find it very helpful > and would like to see it in libavfilter. I do not care as long its GPL.
> On Mar 20, 2017, at 10:13 AM, Paul B Mahol <onemda@gmail.com> wrote: > > On 3/20/17, Dave Rice <dave@dericed.com> wrote: >> On Jan 6, 2017, at 1:34 PM, Gerion Entrup <gerion.entrup.ffdev@flump.de> >> wrote: >>> >>> 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. >> >> Just bumping this thread. I've been using the patch and find it very helpful >> and would like to see it in libavfilter. > > I do not care as long its GPL. Gerion's last post on this thread appears to resolve all review comments but indicated that he would create a FATE test for the filter. Since the patch has been reviewed, I suggest that the missing FATE test could be a later patch and not block consideration of merging the signature filter. As noted, it is written with GPL. Dave Rice
On 3/20/17, Dave Rice <dave@dericed.com> wrote: > >> On Mar 20, 2017, at 10:13 AM, Paul B Mahol <onemda@gmail.com> wrote: >> >> On 3/20/17, Dave Rice <dave@dericed.com> wrote: >>> On Jan 6, 2017, at 1:34 PM, Gerion Entrup <gerion.entrup.ffdev@flump.de> >>> wrote: >>>> >>>> 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. >>> >>> Just bumping this thread. I've been using the patch and find it very >>> helpful >>> and would like to see it in libavfilter. >> >> I do not care as long its GPL. > > Gerion's last post on this thread appears to resolve all review comments but > indicated that he would create a FATE test for the filter. Since the patch > has been reviewed, I suggest that the missing FATE test could be a later > patch and not block consideration of merging the signature filter. As noted, > it is written with GPL. Then wait until someone else appears and like to commit this code.
On Mon, 20 Mar 2017 15:23:10 +0100
Paul B Mahol <onemda@gmail.com> wrote:
> Then wait until someone else appears and like to commit this code.
It would be easier to do for a lazy commit monkey if it was rebased to
current git master:
error: patch failed: Changelog:12
error: Changelog: patch does not apply
error: patch failed: libavfilter/version.h:30
error: libavfilter/version.h: patch does not apply
On Mon, Mar 20, 2017 at 02:31:46PM -0800, Lou Logan wrote: > On Mon, 20 Mar 2017 15:23:10 +0100 > Paul B Mahol <onemda@gmail.com> wrote: > > > Then wait until someone else appears and like to commit this code. > > It would be easier to do for a lazy commit monkey if it was rebased to > current git master: > > error: patch failed: Changelog:12 > error: Changelog: patch does not apply > error: patch failed: libavfilter/version.h:30 > error: libavfilter/version.h: patch does not apply applied btw, often a simple "git am -3" makes git apply a patch with some conflicts markers that otherwise would fail to apply [...]
On Dienstag, 21. März 2017 00:12:52 CET Michael Niedermayer wrote: > On Mon, Mar 20, 2017 at 02:31:46PM -0800, Lou Logan wrote: > > On Mon, 20 Mar 2017 15:23:10 +0100 > > Paul B Mahol <onemda@gmail.com> wrote: > > > > > Then wait until someone else appears and like to commit this code. > > > > It would be easier to do for a lazy commit monkey if it was rebased to > > current git master: > > > > error: patch failed: Changelog:12 > > error: Changelog: patch does not apply > > error: patch failed: libavfilter/version.h:30 > > error: libavfilter/version.h: patch does not apply > > applied > Thank you. I'm currently busy but I try to create a test as soon as I get some time (I already looked into it but don't get the right approach). 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)