[FFmpeg-devel] [PATCH] lavfi: add spp filter.
Clément Bœsch
ubitux at gmail.com
Fri Jun 14 01:25:08 CEST 2013
On Sun, Jun 09, 2013 at 12:27:39AM +0200, Stefano Sabatini wrote:
> On date Saturday 2013-06-08 22:29:23 +0200, Clément Bœsch encoded:
> > TODO: minor bump
> >
> > ---
> >
>
> > I plan to drop mp={fspp,pp7,spp,uspp} filters after this filter is
> > integrated.
>
> For the record, can you explain the relations between the several
> filters, and why spp is the only one which should survive?
>
> That would be highly useful and appreciated.
>
AFAICT, fspp is a "fast" (1d) version of spp, but from the various
feedbacks I had, spp seems to be preferred. pp7 might be interesting, but
I believe it should be merged within spp as a special mode. uspp is a
simple and slow pp (see http://guru.multimedia.cx/pp-vs-spp-filters/ for
comparison with spp), thus not interesting from a practical PoV.
Now this can be discussed more later, since I'll just drop mp=spp for now
and send a patch for the 3 others.
[...]
> > +Apply a simple postprocessing filter that compresses and decompresses the image
> > +at several (or - in the case of @option{quality} level @code{6} - all) shifts
>
> > +and averages the results.
>
> average
>
Fixed.
> I know this is from mplayer docs but as is it is pretty awkward and
> obscure (what is a shift?). Also it could be split in two sentences.
>
I don't really know how to define the shift (it's basically
level=1<<shift); feel free to reword.
[...]
> > +// XXX: share between filters?
>
> what other filters could make use of it?
>
We have various filters with such dithering tabs (gradfun and owdenoise
for instance). They could eventually be merged if they use the same kind
of random.
> > +DECLARE_ALIGNED(8, static const uint8_t, ldither)[8][8] = {
> > + { 0, 48, 12, 60, 3, 51, 15, 63 },
> > + { 32, 16, 44, 28, 35, 19, 47, 31 },
> > + { 8, 56, 4, 52, 11, 59, 7, 55 },
> > + { 40, 24, 36, 20, 43, 27, 39, 23 },
> > + { 2, 50, 14, 62, 1, 49, 13, 61 },
> > + { 34, 18, 46, 30, 33, 17, 45, 29 },
> > + { 10, 58, 6, 54, 9, 57, 5, 53 },
> > + { 42, 26, 38, 22, 41, 25, 37, 21 },
> > +};
> > +
> > +static const uint8_t offset[127][2] = {
> > + {0,0},
> > + {0,0}, {4,4},
> > + {0,0}, {2,2}, {6,4}, {4,6},
> > + {0,0}, {5,1}, {2,2}, {7,3}, {4,4}, {1,5}, {6,6}, {3,7},
> > +
> > + {0,0}, {4,0}, {1,1}, {5,1}, {3,2}, {7,2}, {2,3}, {6,3},
> > + {0,4}, {4,4}, {1,5}, {5,5}, {3,6}, {7,6}, {2,7}, {6,7},
> > +
> > + {0,0}, {0,2}, {0,4}, {0,6}, {1,1}, {1,3}, {1,5}, {1,7},
> > + {2,0}, {2,2}, {2,4}, {2,6}, {3,1}, {3,3}, {3,5}, {3,7},
> > + {4,0}, {4,2}, {4,4}, {4,6}, {5,1}, {5,3}, {5,5}, {5,7},
> > + {6,0}, {6,2}, {6,4}, {6,6}, {7,1}, {7,3}, {7,5}, {7,7},
> > +
> > + {0,0}, {4,4}, {0,4}, {4,0}, {2,2}, {6,6}, {2,6}, {6,2},
> > + {0,2}, {4,6}, {0,6}, {4,2}, {2,0}, {6,4}, {2,4}, {6,0},
> > + {1,1}, {5,5}, {1,5}, {5,1}, {3,3}, {7,7}, {3,7}, {7,3},
> > + {1,3}, {5,7}, {1,7}, {5,3}, {3,1}, {7,5}, {3,5}, {7,1},
> > + {0,1}, {4,5}, {0,5}, {4,1}, {2,3}, {6,7}, {2,7}, {6,3},
> > + {0,3}, {4,7}, {0,7}, {4,3}, {2,1}, {6,5}, {2,5}, {6,1},
> > + {1,0}, {5,4}, {1,4}, {5,0}, {3,2}, {7,6}, {3,6}, {7,2},
> > + {1,2}, {5,6}, {1,6}, {5,2}, {3,0}, {7,4}, {3,4}, {7,0},
> > +};
>
> A note about what these tables are for the future generations would be
> nice.
>
Well that's related to the algorithm, not sure what to say; I added
quality comment for each "block".
> > +
> > +static void hardthresh_c(int16_t dst[64], const int16_t src[64],
> > + int qp, const uint8_t *permutation)
> > +{
> > + int i;
>
> > + int bias = 0; // FIXME
>
> no it will never be fixed
>
Who knows?
> > +
> > + unsigned threshold1 = qp * ((1<<4) - bias) - 1;
> > + unsigned threshold2 = threshold1 << 1;
> > +
> > + memset(dst, 0, 64 * sizeof(dst[0]));
> > + dst[0] = (src[0] + 4) >> 3;
> > +
> > + for (i = 1; i < 64; i++) {
> > + int level = src[i];
> > + if (((unsigned)(level + threshold1)) > threshold2) {
> > + const int j = permutation[i];
> > + dst[j] = (level + 4) >> 3;
> > + }
> > + }
> > +}
> > +
>
> > +static void softthresh_c(int16_t dst[64], const int16_t src[64],
> > + int qp, const uint8_t *permutation)
> > +{
> > + int i;
> > + int bias = 0; //FIXME
> > +
> > + unsigned threshold1 = qp * ((1<<4) - bias) - 1;
> > + unsigned threshold2 = threshold1 << 1;
> > +
> > + memset(dst, 0, 64 * sizeof(dst[0]));
> > + dst[0] = (src[0] + 4) >> 3;
> > +
> > + for (i = 1; i < 64; i++) {
> > + int level = src[i];
> > + if (((unsigned)(level + threshold1)) > threshold2) {
> > + const int j = permutation[i];
> > + if (level > 0) dst[j] = (level - threshold1 + 4) >> 3;
> > + else dst[j] = (level + threshold1 + 4) >> 3;
> > + }
> > + }
> > +}
>
> there could be a clever way to factorize the code with a macro (but
> could be too ugly)
>
I prefer that way
[...]
> > + if (!spp->qp) {
> > + qp_table = av_frame_get_qp_table(in, &qp_stride, &spp->qscale_type);
>
> nit: stride -> linesize? here and below
>
The semantic around qp seems to use the "stride" vocabulary so I kept it
for consistency.
[...]
> > +static int process_command(AVFilterContext *ctx, const char *cmd, const char *args,
> > + char *res, int res_len, int flags)
> > +{
> > + SPPContext *spp = ctx->priv;
> > +
> > + if (!strcmp(cmd, "level")) {
> > + if (!strcmp(args, "max"))
> > + spp->log2_count = MAX_LEVEL;
> > + else
> > + spp->log2_count = av_clip(strtol(args, NULL, 10), 0, MAX_LEVEL);
>
> you could do av_opt_set (and store/restore the old value in case of failure).
>
It sounds simpler to me that way, but feel free to send a patch to change
this.
[...]
> > +#define MAX_LEVEL 6
>
> more descriptive name (level of what?)?
>
It's the quality levels, added a comment
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130614/6b4a2eb3/attachment.asc>
More information about the ffmpeg-devel
mailing list