On Tue, Dec 3, 2013 at 9:43 PM, Anton Khirnov <[email protected]> wrote:
>
> On Thu, 28 Nov 2013 15:03:04 +0100, Vittorio Giovara
> <[email protected]> wrote:
>> ---
>> Changelog | 1 +
>> doc/filters.texi | 19 ++
>> libavfilter/Makefile | 1 +
>> libavfilter/allfilters.c | 1 +
>> libavfilter/version.h | 4 +-
>> libavfilter/vf_framepack.c | 488
>> ++++++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 512 insertions(+), 2 deletions(-)
>> create mode 100644 libavfilter/vf_framepack.c
>>
>> + // check input format
>> + if (s->pix_desc != right_desc) {
>> + av_log(inlink->dst, AV_LOG_ERROR,
>> + "Left and right color spaces differ (%s vs %s).\n",
>> + s->pix_desc->name, right_desc->name);
>> + return AVERROR_INVALIDDATA;
>> + }
>
> The inputs should be treated symmetrically, there is no guarantee in which
> order
> they are initialized. And the check is not needed anyway,
> ff_set_common_formats() makes sure the formats are the same on all pads.
neat, fixed and simplified.
[...]
>> +
>> + if (!interleaved) {
>> + for (i = 0; i < lines; i++) {
>> + memcpy(dstp, leftp, length);
>> + memcpy(dstp + length, rightp, length);
>> +
>> + dstp += dst->linesize[plane];
>> + leftp += s->left->linesize[plane];
>> + rightp += s->right->linesize[plane];
>> + }
>
> Can't av_image_copy() handle the non-interleaved case?
yes, I actually used av_image_copy_plane because that section is
inside a loop with all necessary data, anyway, and because using this
function directly allows me to skip copying any padded data.
>
>> + } else {
>> + for (i = 0; i < lines; i++) {
>> + int j;
>> + int k = 0;
>> +
>> + for (j = 0; j < length; j++) {
>> + dstp[k++] = leftp[j];
>> + dstp[k++] = rightp[j];
>> + }
>> +
>> + dstp += dst->linesize[plane];
>> + leftp += s->left->linesize[plane];
>> + rightp += s->right->linesize[plane];
>> + }
>> + }
>> + }
>> +
>> + return;
>> +}
>> +
>> +static void pack_topbottom_frame(FramepackContext *s,
>> + AVFrame *dst,
>> + int interleaved)
>> +{
>> + int plane, i;
>> + int step = interleaved ? 2 : 1;
>> + int length = s->left->width;
>> + int lines = s->left->height;
>> +
>> + for (plane = 0; plane < s->pix_desc->nb_components; plane++) {
>> + const uint8_t *srcp;
>> + uint8_t *dstp;
>> +
>> + if (plane == 1 || plane == 2) {
>> + length = -(-s->left->width >> s->pix_desc->log2_chroma_w);
>> + lines = -(-s->left->height >> s->pix_desc->log2_chroma_h);
>> + }
>> +
>> + // copy first frame line by line
>> + srcp = s->left->data[plane];
>> + dstp = dst->data[plane];
>> + for (i = 0; i < lines; i ++) {
>> + memcpy(dstp, srcp, length);
>> + dstp += dst->linesize[plane] * step;
>> + srcp += s->left->linesize[plane];
>> + }
>> +
>> + // copy second frame line by line
>> + srcp = s->right->data[plane];
>> + if (interleaved) {
>> + // reset destination pointer and offset one line
>> + dstp = dst->data[plane] + dst->linesize[plane];
>> + }
>> + for (i = 0; i < lines; i ++) {
>> + memcpy(dstp, srcp, length);
>> + dstp += dst->linesize[plane] * step;
>> + srcp += s->right->linesize[plane];
>> + }
>> + }
>
> Can't this be replaced by av_image_copy()?
Yes, I made it similar to the above one.
>
>> +
>> + return;
>> +}
>> +
>> +static void pack_checkerboard_frame(FramepackContext *s, AVFrame *dst)
>> +{
>> + int plane, i;
>> + int length = s->left->width;
>> + int lines = s->left->height;
>> +
>> + for (plane = 0; plane < s->pix_desc->nb_components; plane++) {
>> + const uint8_t *leftp = s->left->data[plane];
>> + const uint8_t *rightp = s->right->data[plane];
>> + uint8_t *dstp = dst->data[plane];
>> +
>> + if (plane == 1 || plane == 2) {
>> + length = -(-s->left->width >> s->pix_desc->log2_chroma_w);
>> + lines = -(-s->left->height >> s->pix_desc->log2_chroma_h);
>> + }
>> +
>> + // alternatively copy one pixel, changing source at each line
>> + for (i = 0; i < lines; i++) {
>> + int k;
>> + for (k = 0; k < length; k += 2) {
>> + dstp[k] = i % 2 ? rightp[k] : leftp[k];
>> + dstp[k + 1] = i % 2 ? leftp[k + 1] : rightp[k + 1];
>> + }
>
> This code looks shady. Should k not run over input length * 2? Or does this
> packing lose half the resolution?
No, this packing loses half the resolution, I'll add a note in
config_output mentioning that.
[...]
>> +static int request_frame(AVFilterLink *outlink)
>> +{
>> + AVFilterContext *ctx = outlink->src;
>> + FramepackContext *s = ctx->priv;
>> + int ret, retl, retr;
>> + AVFrame *out;
>> + static uint64_t double_pts;
>
> static? really? =p
erm, moved to the context instead ^^.
>
>> +
>> + /* get a frame on the left input, on EOF reuse previous */
>> + if (!s->left) {
>> + retl = ff_request_frame(ctx->inputs[LEFT_VIEW]);
>> + if (retl < 0 && retl != AVERROR_EOF)
>> + return retl;
>> + }
>> + /* get a new frame on the right input, on EOF reuse previous */
>> + if (!s->right) {
>> + retr = ff_request_frame(ctx->inputs[RIGHT_VIEW]);
>> + if (retr < 0 && retr != AVERROR_EOF)
>> + return retr;
>> + }
>> + /* when both return values are EOF, it means that both
>> + * videos are fully processed and we can stop */
>> + if (retl == retr && retl == AVERROR_EOF)
>> + return retl;
>> +
>> + if (s->format == AV_STEREO3D_FRAMESEQUENCE) {
>> + AVFrame *cur;
>> + int i;
>> + double_pts = s->left->pts;
>> +
>> + for (i = 0; i < 2; i++) {
>> + cur = i == 0 ? s->left : s->right;
>> + out = av_frame_clone(cur);
>> + if (!out)
>> + return AVERROR(ENOMEM);
>> +
>> + out->pts = double_pts++;
>> + ret = set_side_data(out, s->format);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = ff_filter_frame(outlink, out);
>> + if (ret < 0)
>> + return ret;
>> + }
>> + if (retl != AVERROR_EOF)
>> + av_frame_free(&s->left);
>> + if (retr != AVERROR_EOF)
>> + av_frame_free(&s->right);
>> + return ret;
>> + } else if (s->format == AV_STEREO3D_2D) {
>> + out = av_frame_clone(s->left);
>
> How is that supposed to work?
> The caller connects two streams, but only the left one is actually read?
> Looks awkward/hacky to me, perhaps it'd be better to have a separate filter.
It's a little awkward yeah, it was supposed to be the "default" mode
when no 3d was selected.
I'll go ahead remove it as it breaks the filter structure.
[...]
>> + ret = ff_filter_frame(outlink, out);
>> + if (ret < 0)
>> + return ret;
>> +
>> + // cleanup keeping a valid frame on EOF
>> + if (retl != AVERROR_EOF)
>> + av_frame_free(&s->left);
>> + if (retr != AVERROR_EOF)
>> + av_frame_free(&s->right);
>
> You could move the cleanup before ff_filter_frame, then just return
> ff_filter_frame()
Done.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
