Hi,

Am 19.01.21 um 05:49 schrieb Lynne:
> Jan 19, 2021, 01:07 by borbarak@fb.com:
> 
>> Calculate Spatial Info (SI) and Temporal Info (TI) scores for a video, as defined
>> in ITU-T P.910: Subjective video quality assessment methods for multimedia
>> applications.
>>
>> Update: Fixed bracket style.
>>
> 
> Thanks, looks much neater now.
> 
> 
> 
>> I'm already adding the data to the frame's metadata, is the suggestion to remove the file option altogether?
>>
> 
> Yes. We want to avoid filters having their own file in/out options rather
> than using generic ones.

Updated the patch to apply to git HEAD.
Removed file output.
Made printing summary to console optional.


>> +
>> +#include "libavutil/imgutils.h"
>> +#include "libavutil/internal.h"
>> +#include "libavutil/opt.h"
>> +
>> +#include "avfilter.h"
>> +#include "formats.h"
>> +#include "internal.h"
>> +#include "video.h"
>> +
>> +static const int X_FILTER[9] = {
>> +    1, 0, -1,
>> +    2, 0, -2,
>> +    1, 0, -1
>> +};
>> +
>> +static const int Y_FILTER[9] = {
>> +    1, 2, 1,
>> +    0, 0, 0,
>> +    -1, -2, -1
>> +};
>>
> 
> We have optimized assembly to apply 3x3 matrices. Check out
> libavfilter/x86/vf_convolution.asm:ff_filter_3x3_sse4
>  vf_convolution already applies a sobel filter that way. Maybe
> look into sharing some DSP code with it?

I checked a bit since I also want a common sobel for vf_edgedetect / my patch for vf_blurriness.

For sobel, there is no direct asm implementation. We have a generic filter_3x3 with sse4 optimization. To use sobel with that, you'd need to run two times filter_3x3 plus one pass for gradient calculation.

As another difference, this filter (SITI) does on-the-fly conversion to full-range pixel values during its sobel. While vf_edgedetect / vf_bluriness use an abbreviation for the gradients during its sobel. Which makes them both distinct enough not to fit into a general filter_3x3 or filter_sobel from vf_convolution easily (and more overhead). So I think it's not worth the effort to force them into a common function? (Especially since we don't have a sobel_sse4 or something)

Patch without a common sobel attached.

-Thilo