[FFmpeg-devel] [PATCH 2/2] avfilter/vf_histogram: improve waveform mode

[Marton Balint]

On Sat, 28 Sep 2013, Paul B Mahol wrote:

> On 9/28/13, Marton Balint <cus at passwd.hu> wrote:
>>
>> On Sat, 28 Sep 2013, Paul B Mahol wrote:
>>
>>> On 9/27/13, Marton Balint <cus at passwd.hu> wrote:
>>>> - faster
>>>
>>> How? Are there results and way to reproduce such findings?
>>>
>>
>> Sure, here is one benchmark I just made:
>>
>> time ./ffmpeg -i fate-suite/bink/hol2br.bik -vf \
>> format=yuv444p,histogram=mode=waveform -f null -
>>
>> Old:
>> real	0m0.129s
>> user	0m0.109s
>> sys	0m0.019s
>>
>> New:
>> real    0m0.110s
>> user    0m0.093s
>> sys     0m0.016s
>
> Hmm, if there are changes that make filter faster with same input/output format
> than it belongs again to separate patch.
>

Fine, I will split it.

>>>> - handles more pixel formats
>>>
>>> Well that it is not nice for output, as subsampled pixel format output
>>> would
>>> look worse.
>>>
>>
>> I tried to keep the original design of the filter by keeping the input and
>> the output pixel format the same.
>>
>> Forcing the the output format to an unsubsampled format can be done, but
>> if the source is subsampled, you may not have the resolution to do that
>> without upscaling.
>
> This sentence does not make any sense.
>
> Filter can take YUV411 from input and return YUV444 at output. And no scaling
> of any kind is required.

I meant the following case:
- you got a YUV411 video (512x512) with chroma planes of 128x512.
- with column filter mode you will have a 128x256 waveform.
- but you will have to upscale it to 512x256 for a yuv444 filter output.

So, a yuv444 output also has it's scaling requirements. What do you 
propose?

>>
>> I think the speed benefit of not forcing a colorspace conversion (a
>> software upscaling) and producing sensible output at the same time is
>> great. The filter can actually now work realtime for me on my HD videos.
>
> Just because you use yuv420p instead of yuv444p?
>

Yes.

>>
>> If anybody prefers the waveform of an upscaled chroma plain in full
>> resolution, he can still do that by forcing the input format of the filter
>> to yuv444p.
>
> That adds extra processing which is not needed at all.
>

I meant that the user can force the input formats to the list of the 
currently supported unsubsampled formats, this way the filter will work 
exactly as before - operating always in full resolution, and with an auto 
inserted scaler (extra processing) for subsampled inputs.

>>
>> Look, if it were up to me, I'd only draw the waveform of a single plane
>> but always IN AV_PIX_FMT_GRAY8 format with the proper resolution for the
>> plane. But this would obviously mess up the current design of the
>> histogram filter, so I had two chices. Make the best of the histogram
>> filter, or create a separate waveform filter (which probably will never be
>> accepted into ffmpeg because we already have a waveform filter).
>
> There are already overlay and parade display mode, and adding other modes
> would not be hard.
>
> If you want to have waveform of only say U plane, use histogram in combination
> with extractplanes filter.
>
>>
>>>> - added parameter to mirror the waveform (high values are shown on top
>>>> in
>>>> column mode)
>>>
>>> I do not like when bunch of unrelated changes are put into single patch.
>>> What is point of this mirror thing? Isn't there already filter(s) that do
>>> that?
>>>
>>
>> Mirror mode ensures that high luminance/chrominance values are shown on
>> the top of the waveform in column mode, which is I think how most consumer
>> scopes work.
>
> Good, but than do that in separate patch.
>

Alright.

>>
>> In parade mode you have to mirror the waveform of each plane individually,
>> not the whole frame, so a simple flip filter won't work.
>>
>> I can split the patch to two parts, if you prefer, but I thought since it
>> adds minimal extra complexity and touches new code anyway, it is not
>> necessary.
>
> Patch that changes existing code should change a little as possible, and 
> that is for easy reviewing and/or accepting patches.

Fine.

Regards,
Marton