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

[Paul B Mahol]

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.

>
>>> - 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 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?

>
> 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.

>
> 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.

>
> 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.