> On Mar 20, 2017, at 10:13 AM, Paul B Mahol <one...@gmail.com> wrote:
>
> On 3/20/17, Dave Rice <d...@dericed.com> wrote:
>> On Jan 6, 2017, at 1:34 PM, Gerion Entrup <gerion.entrup.ff...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
