On Wed, 18 Dec 2013 12:51:36 +0100, Vittorio Giovara
<[email protected]> wrote:
> +static void pack_sidebyside_frame(FramepackContext *s,
nit: the name is a bit weird, usually one does either <constant prefix>_$foo or
$foo_<constant suffix>, not <prefix>_$foo_<suffix>
i'd name this pack_horizontal and pack_vertical (or even put each one into a
separate function, they don't share much code). but this is not very important,
so it's up to you, feel free to leave as is.
> + AVFrame *dst,
> + int interleaved)
> +{
> + int plane, i;
> + int length = dst->width / 2;
> + int lines = dst->height;
> +
> + for (plane = 0; plane < s->pix_desc->nb_components; plane++) {
> + const uint8_t *leftp = s->input_views[LEFT]->data[plane];
> + const uint8_t *rightp = s->input_views[RIGHT]->data[plane];
> + uint8_t *dstp = dst->data[plane];
> +
> + if (plane == 1 || plane == 2) {
> + length = -(-dst->width / 2 >> s->pix_desc->log2_chroma_w);
> + lines = -(-dst->height >> s->pix_desc->log2_chroma_h);
more parens here would be appropriate, so it's clear what does the shift apply
to
> + }
> +
> + if (interleaved) {
> + 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->input_views[LEFT]->linesize[plane];
> + rightp += s->input_views[RIGHT]->linesize[plane];
> + }
> + } else {
> + av_image_copy_plane(dst->data[plane], dst->linesize[plane],
> + leftp, s->input_views[LEFT]->linesize[plane],
> + length, lines);
> + av_image_copy_plane(dst->data[plane] + length,
> dst->linesize[plane],
> + rightp,
> s->input_views[RIGHT]->linesize[plane],
> + length, lines);
> + }
Why not av_image_copy()? It'd be simpler and would make the code easier to
generalize e.g. to higher bitdepths.
> + }
> +
> + return;
no need for explicit return here
> +}
> +
> +static void pack_topbottom_frame(FramepackContext *s,
> + AVFrame *dst,
> + int interleaved)
> +{
> + int plane, i;
> + int length = dst->width;
> + int lines = dst->height / 2;
> +
> + for (plane = 0; plane < s->pix_desc->nb_components; plane++) {
> + const uint8_t *leftp = s->input_views[LEFT]->data[plane];
> + const uint8_t *rightp = s->input_views[RIGHT]->data[plane];
> + uint8_t *dstp = dst->data[plane];
> +
> + if (plane == 1 || plane == 2) {
> + length = -(-dst->width >> s->pix_desc->log2_chroma_w);
> + lines = -(-dst->height / 2 >> s->pix_desc->log2_chroma_h);
> + }
> +
> + if (interleaved) {
> + for (i = 0; i < lines; i ++) {
> + memcpy(dstp, leftp, length);
> + leftp += s->input_views[LEFT]->linesize[plane];
> + dstp += dst->linesize[plane];
> +
> + memcpy(dstp, rightp, length);
> + rightp += s->input_views[RIGHT]->linesize[plane];
> + dstp += dst->linesize[plane];
You should be able to handle this with an av_image_copy() with doubled output
linesize
> + }
> + } else {
> + av_image_copy_plane(dst->data[plane],
> + dst->linesize[plane],
> + leftp, s->input_views[LEFT]->linesize[plane],
> + length, lines);
> + av_image_copy_plane(dst->data[plane] + dst->linesize[plane] *
> lines,
> + dst->linesize[plane],
> + rightp,
> s->input_views[RIGHT]->linesize[plane],
> + length, lines);
Again, why not av_image_copy()?
> + }
> + }
> +
> + return;
pointless return again
> +}
> +
> +static av_always_inline void pack_frame(FramepackContext *s, AVFrame *dst)
> +{
> + switch (s->format) {
> + case AV_STEREO3D_COLUMNS:
> + pack_sidebyside_frame(s, dst, 1);
> + break;
> + case AV_STEREO3D_SIDEBYSIDE:
> + pack_sidebyside_frame(s, dst, 0);
> + break;
> + case AV_STEREO3D_LINES:
> + pack_topbottom_frame(s, dst, 1);
> + break;
> + case AV_STEREO3D_TOPBOTTOM:
> + pack_topbottom_frame(s, dst, 0);
> + break;
> + }
> +}
> +
> +static int filter_frame_left(AVFilterLink *inlink, AVFrame *frame)
> +{
> + FramepackContext *s = inlink->dst->priv;
> + s->input_views[LEFT] = frame;
> + return 0;
> +}
> +
> +static int filter_frame_right(AVFilterLink *inlink, AVFrame *frame)
> +{
> + FramepackContext *s = inlink->dst->priv;
> + s->input_views[RIGHT] = frame;
> + return 0;
> +}
> +
> +static int request_frame(AVFilterLink *outlink)
> +{
> + AVFilterContext *ctx = outlink->src;
> + FramepackContext *s = ctx->priv;
> + AVStereo3D *stereo;
> + AVFrame *dst;
> + int ret, i;
> +
> + /* get a frame on the either input, stop as soon as a video ends */
> + for (i = 0; i < 2; i++) {
> + if (!s->input_views[i]) {
> + ret = ff_request_frame(ctx->inputs[i]);
> + if (ret < 0)
> + return ret;
> + }
> + }
> +
> + if (s->format == AV_STEREO3D_FRAMESEQUENCE) {
> + if (s->double_pts == AV_NOPTS_VALUE)
> + s->double_pts = s->input_views[LEFT]->pts;
> +
> + for (i = 0; i < 2; i++) {
> + dst = av_frame_clone(s->input_views[i]);
No need for clone, just use the input frame directly.
> + if (!dst)
> + return AVERROR(ENOMEM);
> + av_frame_free(&s->input_views[i]);
> +
> + /* no need to copy_props as frame is a clone,
> + * just setting correct timestamps here. */
> + dst->pts = s->double_pts++;
> +
> + stereo = av_stereo3d_create_side_data(dst);
> + if (!stereo) {
> + ret = AVERROR(ENOMEM);
> + goto fail;
> + }
> + stereo->type = s->format;
> +
> + ret = ff_filter_frame(outlink, dst);
> + if (ret < 0)
> + return ret;
> + }
> + return ret;
> + } else {
The code below is a bit weird. All of it is executed only if it's spatial (not
temporal) packing, but some of it is stuffed inside the else and some is not.
This might confuse the reader. So either put it all inside the else {} block or
all outside (i prefer the latter myself, but the choice is yours).
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
