[Ffmpeg-devel] Re: Cropping / Padding patches

[Luca Abeni]

Hi Baptiste,

On Tue, 2006-03-14 at 00:35 +0100, Baptiste COUDURIER wrote:
[...]
> > The behaviour of ffmpeg after applying these three patches should not
> > change... I did some tests, and everything is ok. If someone is
> > currently using cropping, padding and rescaling can he/she test the
> > patches and check that they not introduce any regressions?
> 
> I think it changes.
Well, that's not intended :) I'll try to fix it...
> > [...]
> > -    if (ost->video_resample) {
> > +    if (ost->video_pad) {
> >          final_picture = &ost->pict_tmp;
> > [...]
> > +	/* FIXME: PIX_FMT_... */
> > +        if (img_crop((AVPicture *)&picture_pad_temp, (AVPicture *)final_picture, PIX_FMT_YUV420P, ost->padtop, ost->padleft) < 0) {
> >              av_log(NULL, AV_LOG_ERROR, "error cropping picture\n");
> >              goto the_end;
> >          }
> 
> Is cropping needed ?
Here I call img_crop(), but it is not used for cropping... Basically,
padding is performed by copying (or rescaling) the original picture in a
"subset of the final picture", and img_crop() is used for computing such
subset.
 
> > @@ -851,28 +851,28 @@
> >          final_picture = formatted_picture;
> >      }
> >  
> > -        if (enc->pix_fmt != PIX_FMT_YUV420P) {
> > -            int size;
> > +    if (enc->pix_fmt != PIX_FMT_YUV420P) {
> > +        int size;
> >
> > [...]
> >
> >          }
> > +    }
> >      /* duplicates frame if needed */
> >      for(i=0;i<nb_frames;i++) {
> >          AVPacket pkt;
> 
> Isnt that top level ?
Yes.

>  Doing that always convert to yuv420p.
You are right, I introduced a bug... My code always tries to convert
from yuv420p to enc->pix_fmt. The correct version should be
if (enc->pix_fmt != target_pixfmt) {

I'll fix it in the next round of patches, thanks.

> > [...]
> 
> My suggestion is to first fix picture processing to operate on all yuv
> planar formats. Cropping already does, padding should be converted as
> well
I agree, this is on my todo list. I just planned to do it in a second
time.

> I suggest moving fill_pad_region to img_pad in imgconvert.c
This looks reasonable. I also propose to change the function prototype
so that it takes an additional AVPicture* (or AVFrame*, I do not know)
parameter, like the picture_pad_temp used in my patch.
In this way, you can pad by
1) calling img_pad(final_picture, final_height, final_width, padtop, padbottom, padleft, padright, padcolor, &picture_pad_temp)
2) copying (or rescaling) the input picture into picture_pad_temp
3) the padded picture is in final_picture

Michael, what do you think? Is this reasonable?

> I'll try to submit a patch soon, or do you want to do it Luca ?
I'll see what I can do...
But my time to work on this stuff is limited, so if I do not come out
with anything before tomorrow evening feel free to do it yourself.

			Thanks,
				Luca
-- 
_____________________________________________________________________________
Copy this in your signature, if you think it is important:
                               N O    W A R ! ! !

 
 
 --
 Email.it, the professional e-mail, gratis per te: http://www.email.it/f
 
 Sponsor:
 America, Africa, Australia, Asia...con Email Phone Card chiami ovunque spendendo meno di una telefonata interurbana
 Clicca qui: http://adv.email.it/cgi-bin/foclick.cgi?mid=2688&d=14-3