On Thu, Nov 21, 2013 at 12:31:46PM +0100, Vittorio Giovara wrote:
> --- a/Changelog
> +++ b/Changelog
> @@ -49,6 +49,7 @@ version 10:
> - dar variable in the scale filter now returns the actual DAR (i.e. a * sar)
> - VP9 decoder
> - codec level stereoscopic metadata handling
> +- new framepack filter
Everything in this file is new, or was there a framepack filter before?
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -1252,6 +1252,20 @@ frames with a negative PTS.
> +@section frampack
fram_e_pack
> +Packs two different video streams into a stereoscopic video. The two videos
> should have the same size and framerate; you may conveniently adjust those
> values with @ref{scale} and @ref{fps} filters. If one video lasts longer than
> the other the last frame is repeated.
s/with/with the/
s/the other/the other,/
Please keep line length reasonable, same below.
> +@item format
> +Desired framepacking format. Supported values are @var{2d} (default),
> @var{sbs}, @var{tab}, @var{lines}, @var{columns}, @var{frameseq},
> @var{check}. See a detailed format descritpion in libavutil/stereo3d.h.
@file{libavutil/stereo3d.h}
> --- /dev/null
> +++ b/libavfilter/vf_framepack.c
> @@ -0,0 +1,505 @@
> +
> +/**
> + * @file
> + * generate a frame packed video
Now that I think about it I'm not sure what a "frame packed" video is.
You could use a few more words here. Also, capitalize and end in a
period.
> +#include "libavutil/common.h"
> +#include "libavutil/eval.h"
> +#include "libavutil/rational.h"
> +#include "libavutil/pixdesc.h"
> +#include "libavutil/imgutils.h"
> +#include "libavutil/mathematics.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/stereo3d.h"
nit: order
> +typedef struct {
> + const AVClass *class;
> +
> + const AVPixFmtDescriptor *pix_desc; ///< the agreed pixel format
> +
> + enum AVStereo3DType format; ///< the frame packed output
> +
> + AVFrame *left; ///< the left input frame
> + AVFrame *right; ///< the right input frame
> +} FramepackContext;
No anonymous structs please, they cause trouble in headers, we should
try to eliminate them overall.
> +static int config_input_left(AVFilterLink *inlink)
> +{
> + AVFilterContext *ctx = inlink->dst;
> + FramepackContext *s = inlink->dst->priv;
nit: align
> +static int config_input_right(AVFilterLink *inlink)
> +{
> + AVFilterContext *ctx = inlink->dst;
> + FramepackContext *s = inlink->dst->priv;
same
> +static int config_output(AVFilterLink *outlink)
> +{
> + AVFilterContext *ctx = outlink->src;
> + FramepackContext *s = outlink->src->priv;
> +
> + int width, height;
> + AVRational time_base;
> +
> + // normal case, same as input
> + width = ctx->inputs[LEFT_VIEW]->w;
> + height = ctx->inputs[LEFT_VIEW]->h;
> + time_base = ctx->inputs[LEFT_VIEW]->time_base;
I suggest merging declaration and initialization.
> + av_log(ctx, AV_LOG_ERROR,
> + "Left and right sizes differ (%dx%d vs %dx%d).\n",
> + width, height, ctx->inputs[RIGHT_VIEW]->w,
> ctx->inputs[RIGHT_VIEW]->h);
nit: long line
> + 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);
nit: align
> + 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);
same
> + 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);
same
> + // alternatively copy one pixel, changing source at each line
> + for (i = 0; i < lines; i++) {
> + int k = 0;
> + for (k = 0; k < length; k += 2) {
Setting k to 0 is unnecessary.
> +static int set_side_data(AVFrame *out, enum AVStereo3DType format)
> +{
> + AVFrameSideData *side_data;
> + AVStereo3D *stereo;
> +
> + stereo = av_stereo3d_alloc();
> + if (!stereo)
> + return AVERROR(ENOMEM);
> +
> + stereo->type = format;
> + if (format == AV_STEREO3D_CHECKERS)
> + stereo->info |= AV_STEREO3D_SAMPLE_QUINCUNX;
> +
> + side_data = av_frame_new_side_data(out,
> + AV_FRAME_DATA_STEREO3D,
> + sizeof(AVStereo3D));
> + if (!side_data)
> + return AVERROR(ENOMEM);
I think you leak stereo if you return here.
> +static const AVOption options[] = {
> + { "format", "Frame pack output format", OFFSET(format), AV_OPT_TYPE_INT,
> + {.i64 = AV_STEREO3D_2D }, 0, INT_MAX, .flags = V, .unit = "format" },
spaces inside {}
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
