[FFmpeg-devel] [PATCH 3/5] Renamed reinterlace to tinterlace
Vasile Toncu
vasile.toncu at tremend.com
Mon Aug 13 01:02:22 EEST 2018
Hello,
I have updated patch 3 according to review, removed all code related to the
new flags (MERGE_TFF, MERGE_BFF) and threading functionality.
On Sat, Jul 28, 2018 at 5:04 PM, Thomas Mundt <tmundt75 at gmail.com> wrote:
> Hi,
>
> 2018-07-24 12:17 GMT+02:00 Vasile Toncu <vasile.toncu at tremend.com>:
>
> > Fixed tabs.
> > Thank you for the feedback.
> >
>
> > {....}
> > + case MODE_INTERLEAVE_BOTTOM:
> > + case MODE_INTERLEAVE_TOP:
> > + y = y * 2;
> > +
> > + if (tinterlace->flags & FLAG_VLPF || tinterlace->flags &
> > FLAG_CVLPF) {
> > +
> > + int lines, cols, cvlfp;
> > + AVFrame *from_frame;
> > + uint8_t *from, *to;
> > + int from_step, to_step;
> > +
> > + lines = (tinterlace_mode == MODE_INTERLEAVE_TOP) ? (2 *
> > out->height / y + 1) / 2 : (2 * out->height / y + 0) / 2;
> > + cols = out->width / x;
> > + from_frame = first;
> > + from = from_frame->data[plane];
> > + to = out->data[plane];
> > +
> > + if (tinterlace_mode == MODE_INTERLEAVE_BOTTOM) {
> > + from = from + from_frame->linesize[plane];
> > + to = to + out->linesize[plane];
> > + }
> > +
> > + from_step = 2 * from_frame->linesize[plane];
> > + to_step = 2 * out->linesize[plane];
> > +
> > + // when i = lines - aka first line
> > + tinterlace->lowpass_line(to, cols, from,
> > from_frame->linesize[plane], 0, clip_max);
> > + to += to_step;
> > + from += from_step;
> > +
> > + cvlfp = !!(tinterlace->flags & FLAG_CVLPF);
> > + if (cvlfp) {
> > + tinterlace->lowpass_line(to, cols, from,
> > from_frame->linesize[plane], 0, clip_max);
> > + to += to_step;
> > + from += from_step;
> > + }
> > +
> > + for (i = lines - 2 - 2 * cvlfp; i; i--) {
> > + tinterlace->lowpass_line(to, cols, from,
> > from_frame->linesize[plane], -from_frame->linesize[plane], clip_max);
> > + to += to_step;
> > + from += from_step;
> > + }
> > +
> > + // when i == 1 - aka last line
> > + tinterlace->lowpass_line(to, cols, from, 0,
> > -from_frame->linesize[plane], clip_max);
> > + to += to_step;
> > + from += from_step;
> > +
> > + if (cvlfp) {
> > + tinterlace->lowpass_line(to, cols, from, 0,
> > -from_frame->linesize[plane], clip_max);
> > + to += to_step;
> > + from += from_step;
> > + }
> > +
> > +
> > + lines = (tinterlace_mode == MODE_INTERLEAVE_BOTTOM) ? ((2 *
> > out->height / y) + 1) / 2 : (2 * out->height / y + 0) / 2;
> > + cols = out->width / x;
> > + from_frame = second;
> > + from = from_frame->data[plane];
> > + to = out->data[plane];
> > +
> > + if (tinterlace_mode == MODE_INTERLEAVE_TOP) {
> > + from = from + from_frame->linesize[plane];
> > + to = to + out->linesize[plane];
> > }
> > +
> > + from_step = 2 * from_frame->linesize[plane];
> > + to_step = 2 * out->linesize[plane];
> > +
> > + // when i = lines
> > + tinterlace->lowpass_line(to, cols, from,
> > from_frame->linesize[plane], 0, clip_max);
> > + to += to_step;
> > + from += from_step;
> > +
> > + if (cvlfp) {
> > + tinterlace->lowpass_line(to, cols, from,
> > from_frame->linesize[plane], 0, clip_max);
> > + to += to_step;
> > + from += from_step;
> > + }
> > +
> > +
> > + for (i = lines - 2 - 2 * cvlfp; i; i--) {
> > + tinterlace->lowpass_line(to, cols, from,
> > from_frame->linesize[plane], -from_frame->linesize[plane], clip_max);
> > + to += to_step;
> > + from += from_step;
> > + }
> > +
> > + // when i == 1
> > + tinterlace->lowpass_line(to, cols, from, 0,
> > -from_frame->linesize[plane], clip_max);
> > + to += to_step;
> > + from += from_step;
> > +
> > + if (cvlfp) {
> > + tinterlace->lowpass_line(to, cols, from, 0,
> > -from_frame->linesize[plane], clip_max);
> > + to += to_step;
> > + from += from_step;
> > + }
> >
>
> Compared to the one simple "for"-loop in the GPL version, this looks very
> complicated. Maybe it´s ok here to just keep the original code since it has
> been modified in the past anyway.
> But at least you need to factor it out for not having the same code two
> times in a row.
>
> {....}
> > + case MODE_MERGE_TFF:
> > + case MODE_MERGE_BFF:
> > + offset1 = (tinterlace_mode == MODE_MERGE_TFF) ? 0 :
> > out->linesize[plane];
> > + offset2 = (tinterlace_mode == MODE_MERGE_TFF) ?
> > out->linesize[plane] : 0;
> > +
> > + av_image_copy_plane(out->data[plane] + offset1, 2 *
> > out->linesize[plane],
> > + first->data[plane], first->linesize[plane], first->width /
> x,
> > first->height / y);
> > + av_image_copy_plane(out->data[plane] + offset2, 2 *
> > out->linesize[plane],
> > + second->data[plane], second->linesize[plane], second->width
> /
> > x, second->height / y);
> > + break;
> >
>
> These are new functions, independent from the license change. Please put
> them in a separate patch together with documentation and fate tests.
>
>
> > {....}
> > +static TInterlaceThreadData *get_TInterlaceThreadData(AVFrame *out,
> > AVFrame *first, AVFrame *second,
> > + int plane, TInterlaceContext *tinterlace,
> > + int scale_w_plane12_factor,
> > + int scale_h_plane12_factor)
> > {
> > - AVFilterContext *ctx = inlink->dst;
> > - AVFilterLink *outlink = ctx->outputs[0];
> > - TInterlaceContext *tinterlace = ctx->priv;
> > - AVFrame *cur, *next, *out;
> > - int field, tff, ret;
> > -
> > - av_frame_free(&tinterlace->cur);
> > - tinterlace->cur = tinterlace->next;
> > - tinterlace->next = picref;
> > -
> > - cur = tinterlace->cur;
> > - next = tinterlace->next;
> > - /* we need at least two frames */
> > - if (!tinterlace->cur)
> > - return 0;
> > -
> > - switch (tinterlace->mode) {
> > - case MODE_MERGEX2: /* move the odd frame into the upper field of the
> > new image, even into
> > - * the lower field, generating a double-height
> > video at same framerate */
> > - case MODE_MERGE: /* move the odd frame into the upper field of the
> > new image, even into
> > - * the lower field, generating a double-height video at half
> > framerate */
> > - out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
> > + TInterlaceThreadData *rtd = &((TInterlaceThreadData
> > *)tinterlace->thread_data)[plane];
> > +
> > + if (!rtd)
> > + return rtd;
> > +
> > + rtd->out = out;
> > + rtd->first = first;
> > + rtd->second = second;
> > + rtd->plane = plane;
> > + rtd->tinterlace = tinterlace;
> > + rtd->scale_h_plane12_factor = scale_h_plane12_factor;
> > + rtd->scale_w_plane12_factor = scale_w_plane12_factor;
> > +
> > + return rtd;
> > +}
> >
>
> Threading support is also independent from the lincense change and should
> be added by a separate patch.
>
> {....}
> > AVFilter ff_vf_tinterlace = {
> > .name = "tinterlace",
> > - .description = NULL_IF_CONFIG_SMALL("Perform temporal field
> > interlacing."),
> > + .description = NULL_IF_CONFIG_SMALL("Various interlace frame
> > manipulations"),
> > .priv_size = sizeof(TInterlaceContext),
> > + .init = init,
> > .uninit = uninit,
> > .query_formats = query_formats,
> > .inputs = tinterlace_inputs,
> > .outputs = tinterlace_outputs,
> > .priv_class = &tinterlace_class,
> > + .flags = AVFILTER_FLAG_SLICE_THREADS |
> > AVFILTER_FLAG_SUPPORT_TIMELINE_GENERIC,
> > };
> >
> > -
> > AVFilter ff_vf_interlace = {
> > .name = "interlace",
> > .description = NULL_IF_CONFIG_SMALL("Convert progressive video
> into
> > interlaced."),
> > .priv_size = sizeof(TInterlaceContext),
> > + .init = init,
> > .uninit = uninit,
> > .query_formats = query_formats,
> > .inputs = tinterlace_inputs,
> >
>
> When you add new flags to vf_tinterlace, please also add them to
> vf_interlace.
>
> {....}
> > +#elif /* CONFIG_GPL */
> > +
> > +av_cold void ff_tinterlace_init_x86(ReInterlaceContext *s) {}
> >
>
> ReInterlace?
>
> +
> > +#endif /* CONFIG_GPL */
> > --
> > 2.17.1
> >
>
> The rest looks ok to me. Compiler warnings are gone and fate tests passes.
> However, as stated before, I´m not able to judge the requirements for the
> license change.
>
> Regards,
> Thomas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Patch-3-Move-reinterlace-to-tinterlace.patch
Type: text/x-patch
Size: 89504 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180813/c2360e08/attachment.bin>
More information about the ffmpeg-devel
mailing list