On Wed, Dec 18, 2013 at 1:25 PM, Anton Khirnov <[email protected]> wrote:
>
> 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.
No no consistency is important, I'll change it to something more verbose too.
>> + } 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.
I don't know about higher bitdepths, but I used av_image_copy_plane()
because I already had all data for it (like image pitch, and correct
plane height) and because av_image_copy_plane() doesn't copy all the
padding which seemed a waste.
Would you still recommend using av_image_copy()?
>> +
>> +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
Actually yes!
>
>> + }
>> + } 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()?
>
See above.
>> + 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.
>
I'm changing pts and adding side data though, so I'd have to add
needs_writable on the input pad.
However I'd only need that for this temporal packing so I'd prefer not
to enable it for the other modes.
>> + 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).
Yes, I could feel something weird reading the code, but I couldn't put
my finger on it. Having everything inside the else {} block makes
things more understandable.
>
> --
> Anton Khirnov
Thank you for these comments,
Vittorio
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
