[FFmpeg-devel] [PATCH] histogram filter
Paul B Mahol
onemda at gmail.com
Sat Feb 9 12:53:39 CET 2013
On 2/8/13, Stefano Sabatini <stefasab at gmail.com> wrote:
> On date Thursday 2013-02-07 13:01:23 +0000, Paul B Mahol encoded:
>> Signed-off-by: Paul B Mahol <onemda at gmail.com>
>> ---
>> doc/filters.texi | 49 ++++++++
>> libavfilter/Makefile | 1 +
>> libavfilter/allfilters.c | 1 +
>> libavfilter/vf_histogram.c | 299
>> +++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 350 insertions(+)
>> create mode 100644 libavfilter/vf_histogram.c
>>
>> diff --git a/doc/filters.texi b/doc/filters.texi
>> index e047852..2925cbe 100644
>> --- a/doc/filters.texi
>> +++ b/doc/filters.texi
>> @@ -3098,6 +3098,55 @@ the histogram. Possible values are @code{none},
>> @code{weak} or
>> @code{strong}. It defaults to @code{none}.
>> @end table
>>
>> + at section histogram
>> +
>> +Compute and draw an histogram.
>
> Compute and draw a color distribution histogram for the input video.
>
>> +
>> +Computed histogram is representation of distribution of color components
>
> The computed histogram ... is a representation ...
>
>> +in an image.
>
> You may mention that the histogram is relative to the input format,
> and maybe provide an example showing how to force a format in the
> examples section.
>
>> +
>> +The filter accepts the following named parameters:
>> +
>> + at table @option
>> + at item mode
>> +Set histogram mode.
>> +
>> +It accepts the following values:
>> + at table @samp
>> + at item levels
>> +standard histogram that display color components
>> +distribution in an image.
>> + at item color
>> +chroma values in vectorscope, if brighter more
>> +such chroma values are distributed in image.
>> + at item color2
>> +chroma values in vectorscope, similar as color
>> +but actual values are displayed.
>> + at item waveform
>> +per-line luminance graph.
>> + at end table
>> +Default value is @code{levels}.
>
> I would like a more accurate definition of this, it is really hard to
> understand what they really do if not inspecting the code or the
> output.
>
> Copy&pasting from avisynth docs would be fine, I don't mind to fix
> eventual typos later, feel free to skip if this bores you to death,
> I'll provide docs later.
>
>> +
>> + at item level_height
>> +Set height of level in @code{levels}. Default value is @code{200}.
>> +Allowed range is [50, 2048].
>> +
>> + at item scale_height
>> +Set height of color scale in @code{levels}. Default value is @code{10}.
>> +Allowed range is [0, 40].
>
> BTW do you think would be possible to select just a component?, for
> example if I want to show only one component (that would be possible
> by some weird filter combination, but having this in the filter would
> be simpler for the user).
>
>> +
>> + at subsection Examples
>> +
>> + at itemize
>> +
>> + at item
>> +Calculate and draw histogram:
>> + at example
>> +ffplay -i input -vf histogram
>> + at end example
>> +
>> + at end itemize
>> +
>> @section hqdn3d
>>
>> High precision/quality 3d denoise filter. This filter aims to reduce
>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
>> index 938b183..a9dfc16 100644
>> --- a/libavfilter/Makefile
>> +++ b/libavfilter/Makefile
>> @@ -122,6 +122,7 @@ OBJS-$(CONFIG_GEQ_FILTER) +=
>> vf_geq.o
>> OBJS-$(CONFIG_GRADFUN_FILTER) += vf_gradfun.o
>> OBJS-$(CONFIG_HFLIP_FILTER) += vf_hflip.o
>> OBJS-$(CONFIG_HISTEQ_FILTER) += vf_histeq.o
>> +OBJS-$(CONFIG_HISTOGRAM_FILTER) += vf_histogram.o
>> OBJS-$(CONFIG_HQDN3D_FILTER) += vf_hqdn3d.o
>> OBJS-$(CONFIG_HUE_FILTER) += vf_hue.o
>> OBJS-$(CONFIG_IDET_FILTER) += vf_idet.o
>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
>> index 47158f9..0def0b4 100644
>> --- a/libavfilter/allfilters.c
>> +++ b/libavfilter/allfilters.c
>> @@ -116,6 +116,7 @@ void avfilter_register_all(void)
>> REGISTER_FILTER(GRADFUN, gradfun, vf);
>> REGISTER_FILTER(HFLIP, hflip, vf);
>> REGISTER_FILTER(HISTEQ, histeq, vf);
>> + REGISTER_FILTER(HISTOGRAM, histogram, vf);
>> REGISTER_FILTER(HQDN3D, hqdn3d, vf);
>> REGISTER_FILTER(HUE, hue, vf);
>> REGISTER_FILTER(IDET, idet, vf);
>> diff --git a/libavfilter/vf_histogram.c b/libavfilter/vf_histogram.c
>> new file mode 100644
>> index 0000000..4659bd5
>> --- /dev/null
>> +++ b/libavfilter/vf_histogram.c
>> @@ -0,0 +1,299 @@
>> +/*
>> + * Copyright (c) 2012 Paul B Mahol
>> + *
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> 02110-1301 USA
>> + */
>> +
>> +#include "libavutil/avassert.h"
>> +#include "libavutil/opt.h"
>> +#include "libavutil/parseutils.h"
>> +#include "libavutil/pixdesc.h"
>> +#include "avfilter.h"
>> +#include "formats.h"
>> +#include "internal.h"
>> +#include "video.h"
>> +
>> +enum HistogramMode {
>> + LEVELS,
>> + WAVEFORM,
>> + COLOR,
>> + COLOR2,
>> + MODE_NB
>> +};
>
> Nit: I suggest to add some prefix MODE_ to clarify the context, and
> avoid possible future conflicts.
>
>> +
>> +typedef struct HistogramContext {
>> + const AVClass *class; ///< AVClass context for log and
>> options purpose
>> + enum HistogramMode mode;
>
>> + unsigned histogram[4][256];
>> + unsigned max_hval[4];
>
> a short doxy here may help
>
>> + int ncomp;
>> + const uint8_t *bg_color;
>> + const uint8_t *fg_color;
>> + int level_height;
>> + int scale_height;
>> +} HistogramContext;
>> +
>> +#define OFFSET(x) offsetof(HistogramContext, x)
>> +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
>> +
>> +static const AVOption histogram_options[] = {
>> + { "mode", "set histogram mode", OFFSET(mode), AV_OPT_TYPE_INT,
>> {.i64=LEVELS}, 0, MODE_NB-1, FLAGS, "mode"},
>> + { "levels", "standard histogram", 0, AV_OPT_TYPE_CONST,
>> {.i64=LEVELS}, 0, 0, FLAGS, "mode" },
>> + { "waveform", "per-line luminance graph", 0, AV_OPT_TYPE_CONST,
>> {.i64=WAVEFORM}, 0, 0, FLAGS, "mode" },
>> + { "color", "chroma values in vectorscope", 0, AV_OPT_TYPE_CONST,
>> {.i64=COLOR}, 0, 0, FLAGS, "mode" },
>> + { "color2", "chroma values in vectorscope", 0, AV_OPT_TYPE_CONST,
>> {.i64=COLOR2}, 0, 0, FLAGS, "mode" },
>> + { "level_height", "set level height", OFFSET(level_height),
>> AV_OPT_TYPE_INT, {.i64=200}, 50, 2048, FLAGS},
>> + { "scale_height", "set scale height", OFFSET(scale_height),
>> AV_OPT_TYPE_INT, {.i64=10}, 0, 40, FLAGS},
>> + { NULL },
>> +};
>> +
>> +AVFILTER_DEFINE_CLASS(histogram);
>> +
>> +static av_cold int init(AVFilterContext *ctx, const char *args)
>> +{
>> + HistogramContext *h = ctx->priv;
>> + int ret;
>> +
>> + h->class = &histogram_class;
>> + av_opt_set_defaults(h);
>> +
>> + if ((ret = (av_set_options_string(h, args, "=", ":"))) < 0)
>
> Here you could use av_opt_set_string() and select a shorthand for the
> mode.
>
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static const enum AVPixelFormat color_pix_fmts[] = {
>> + AV_PIX_FMT_YUV444P, AV_PIX_FMT_YUVA444P, AV_PIX_FMT_YUVJ444P,
>> + AV_PIX_FMT_NONE
>> +};
>> +
>> +static const enum AVPixelFormat levels_pix_fmts[] = {
>> + AV_PIX_FMT_YUV444P, AV_PIX_FMT_YUVA444P, AV_PIX_FMT_YUVJ444P,
>> + AV_PIX_FMT_GRAY8, AV_PIX_FMT_GBRP, AV_PIX_FMT_NONE
>> +};
>> +
>> +static int query_formats(AVFilterContext *ctx)
>> +{
>> + HistogramContext *h = ctx->priv;
>> + const enum AVPixelFormat *pix_fmts;
>> +
>> + switch (h->mode) {
>> + case LEVELS:
>> + pix_fmts = levels_pix_fmts;
>> + break;
>> + case WAVEFORM:
>> + case COLOR:
>> + case COLOR2:
>> + pix_fmts = color_pix_fmts;
>> + break;
>> + default:
>> + av_assert0(0);
>> + }
>> +
>> + ff_set_common_formats(ctx, ff_make_format_list(pix_fmts));
>> +
>> + return 0;
>> +}
>> +
>> +static const uint8_t black_yuva_color[4] = { 0, 127, 127, 255 };
>> +static const uint8_t black_gbrp_color[4] = { 0, 0, 0, 255 };
>> +static const uint8_t white_yuva_color[4] = { 255, 127, 127, 255 };
>> +static const uint8_t white_gbrp_color[4] = { 255, 255, 255, 255 };
>> +
>> +static int config_input(AVFilterLink *inlink)
>> +{
>> + HistogramContext *h = inlink->dst->priv;
>> + const AVPixFmtDescriptor *desc =
>> av_pix_fmt_desc_get(inlink->format);
>> +
>> + h->ncomp = desc->nb_components;
>> +
>> + switch (inlink->format) {
>> + case AV_PIX_FMT_GBRP:
>> + h->bg_color = black_gbrp_color;
>> + h->fg_color = white_gbrp_color;
>> + break;
>> + default:
>> + h->bg_color = black_yuva_color;
>> + h->fg_color = white_yuva_color;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int config_output(AVFilterLink *outlink)
>> +{
>> + AVFilterContext *ctx = outlink->src;
>> + HistogramContext *h = ctx->priv;
>> +
>> + switch (h->mode) {
>> + case LEVELS:
>> + outlink->w = 256;
>> + outlink->h = (h->level_height + h->scale_height) * h->ncomp;
>> + break;
>> + case WAVEFORM:
>> + outlink->w = 256;
>> + break;
>> + case COLOR:
>> + case COLOR2:
>> + outlink->h = outlink->w = 256;
>> + break;
>> + default:
>> + av_assert0(0);
>> + }
>
> maybe you could add some consistency checks here in case levels
> options are specified but mode != LEVELS.
>
>> +
>> + outlink->sample_aspect_ratio = (AVRational){1,1};
>> +
>> + return 0;
>> +}
>> +
>> +static int filter_frame(AVFilterLink *inlink, AVFilterBufferRef *in)
>> +{
>> + HistogramContext *h = inlink->dst->priv;
>> + AVFilterContext *ctx = inlink->dst;
>> + AVFilterLink *outlink = ctx->outputs[0];
>> + AVFilterBufferRef *out;
>> + const uint8_t *src;
>> + int i, j, k, l, ret;
>> +
>> + out = ff_get_video_buffer(outlink, AV_PERM_WRITE, outlink->w,
>> outlink->h);
>> + if (!out) {
>> + avfilter_unref_bufferp(&in);
>> + return AVERROR(ENOMEM);
>> + }
>> +
>> + out->pts = in->pts;
>> + out->pos = in->pos;
>
> avfilter_copy_buffer_ref_props() may be safer in case we add more
> params (e.g. duration), or me may add a shallower macro/function to
> copy only minimal stuff.
>
>> +
>> + for (k = 0; k < h->ncomp; k++)
>> + for (i = 0; i < outlink->h; i++)
>> + memset(out->data[k] + i * out->linesize[k], h->bg_color[k],
>> outlink->w);
>> +
>> + switch (h->mode) {
>> + case LEVELS:
>
> please add a note before each block explaining what it does, they are
> really valuable when reading/debugging the code for the first time
>
>> + for (k = 0; k < h->ncomp; k++) {
>> + for (i = 0; i < in->video->h; i++) {
>
>> + int lsize = i * in->linesize[k];
>
> "lsize" is not a linesize, so the name is a bit confusing.
>
>> + for (j = 0; j < in->video->w; j++) {
>
>> + src = in->data[k] + lsize;
>
> is this supposed to change in the inner loop?
>
>> + h->histogram[k][src[j]]++;
>> + }
>
> I suggest (untested):
>
> for (k = 0; k < h->ncomp; k++) {
> for (i = 0; i < in->video->h; i++) {
> src = in->data[k] + i * in->linesize[k];
> for (j = 0; j < in->video->w; j++)
> h->histogram[k][src[j]]++;
> }
> }
>
>> + }
>> + }
>> +
>> + for (k = 0; k < h->ncomp; k++)
>> + for (i = 0; i < 256; i++)
>> + h->max_hval[k] = FFMAX(h->max_hval[k],
>> h->histogram[k][i]);
>> +
>> + for (k = 0; k < h->ncomp; k++) {
>> + int start = k * (h->level_height + h->scale_height);
>> + for (i = 0; i < outlink->w; i++) {
>> + int col_height = h->level_height -
>> (float)h->histogram[k][i] / h->max_hval[k] * h->level_height;
>> +
>> + for (j = h->level_height - 1; j >= col_height; j--)
>> + for (l = 0; l < h->ncomp; l++)
>> + out->data[l][(j + start) * out->linesize[l] + i]
>> = h->fg_color[l];
>> + for (j = h->level_height + h->scale_height - 1; j >=
>> h->level_height; j--)
>> + out->data[k][(j + start) * out->linesize[0] + i] =
>> i;
>> + }
>> + }
>> +
>> + for (k = 0; k < h->ncomp; k++) {
>> + memset(h->histogram[k], 0, 256 * sizeof(unsigned));
>> + h->max_hval[k] = 0;
>> + }
>
> I suppose you could merge the external loops on k
>
>> + break;
>> + case WAVEFORM:
>> + for (i = 0; i < inlink->h; i++) {
>
>> + int ow = i * out->linesize[0];
>> + int iw = i * in->linesize[0];
>> + for (j = 0; j < inlink->w; j++)
>> + out->data[0][ow + in->data[0][iw + j]] = 255;
>
> again i'm confused by the variable names choice, i suggest:
>
> for (i = 0; i < inlink->h; i++) {
> src = in ->data[0] + i *in ->linesize;
> dst = out->data[0] + i *out->linesize;
> for (j = 0; j < inlink->w; j++)
> dst[src[j]] = 255;
>
> Also it would be more interesting if you could also set an intensity
> based on the number of value hits.
>
> [...]
>> +AVFilter avfilter_vf_histogram = {
>> + .name = "histogram",
>> + .description = NULL_IF_CONFIG_SMALL("Compute and draw an
>> histogram."),
>
> "a histogram"
>
> [...]
I fixed most of things I could and pushed. Feel free to improve it as you like.
More information about the ffmpeg-devel
mailing list