Hi,
2018-07-24 12:17 GMT+02:00 Vasile Toncu <vasile.to...@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@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
