[MPlayer-dev-eng] [PATCH] fix aspect ratio bug for MPEG1 PAL 16/9

[Giacomo Comes]

On Thu, Aug 24, 2006 at 12:41:35PM +0300, Ivan Kalvachev wrote:
> 2006/8/23, Giacomo Comes <comes at naic.edu>:
> >I have found a small bug of libavcodec during the creation of mpeg1 video.
> >Here is the description of the problem.
> >
> >The following command is a simple one for creating MPEG1 PAL video with 
> >16/9
> >aspect ratio:
> >
> >mencoder -of mpeg -mpegopts format=xvcd -ovc lavc -lavcopts 
> >vcodec=mpeg1video:vrc_buf_size=327:vrc_minrate=1152:vrc_maxrate=1152:keyint=15:vmax_b_frames=2:aspect=16/9:vbitrate=1152 -o test.mpg source.avi -ofps 25 -vf scale=352:288,harddup -oac lavc -lavcopts acodec=mp2:abitrate=224
> >
> >The MPEG1 standard says that the values for the 4 bit pel_aspect_ratio 
> >field are:
> >
> >pel  w/h
> > 3  0.7031   16:9 625 lines
> > 6  0.8437   16:9 525 lines
> > 8  0.9375    4:3 625 lines
> >12  1.1250    4:3 525 lines
> >
> >libavcodec set the correct value for the aspect ratio in all cases except 
> >the first
> >one. MPEG1 PAL 16/9 instead of having aspect 3 has aspect 2.
> >
> >The code that set the aspect ratio in the function 
> >mpeg1_encode_sequence_header
> >in the file mpeg12.c is:
> >
> >    for(i=1; i<15; i++){
> >        float error= aspect_ratio;
> >        if(s->codec_id == CODEC_ID_MPEG1VIDEO || i <=1)
> >            error-= 1.0/mpeg1_aspect[i];
> >        else
> >            error-= av_q2d(mpeg2_aspect[i])*s->height/s->width;
> >        error= ABS(error);
> >        if(error < best_aspect_error){
> >            best_aspect_error= error;
> >            s->aspect_ratio_info= i;
> >        }
> >    }
> >
> >The code itself is not wrong: for PAL 16/9, the aspect ratio that gives 
> >the smallest
> >best_aspect_error is really 2 (2 -> 0.030235, 3 -> 0.032272), but the 
> >standard
> >says that in this case the aspect has to be 3. Other tools, like 
> >mjpegtools,
> >follow the standard and set 3 as aspect ratio for MPEG1 PAL 16/9.
> >
> >The attached patch check for the specific case (MPEG1 PAL 16/9) and set 
> >aspect 3,
> >leaving all the other cases to the for loop.
> >
> >If you think the patch is correct and should be committed, please forward 
> >to the
> >appropriate list.
> 
> This part of mplayer is imported from FFmpeg project and you have to
> post it to ffmpeg-dev maillist.

I know, that's why I sayd to forward to the appropriate list. I don't 
follow ffmpeg-dev so I posted here.

> 
> I'm sure your patch won't be accepted. It always forces #3 when
> aspect=16/9 is required despite that according to your reasoning #6 is
> also 16/9 .
> There is no code to detect PAL vs NTSC mode at all. Nor it takes image
> size in calculations.

No. The patch forces aspect 3 only for PAL 16/9. If you have NTSC 16/9
the aspect will be 6 (the for cicle will be executed).
The code "ABS(aspect_ratio-16.0/11)" actually check for PAL 16/9.
The value of the variable aspect_ratio is the aspect ratio multiplied 
by the ratio of the movie size.


> 
> However I'm not completely sure that your problem is mpeg12.c encoder
> fault.  The standard says that the encoder have to pick the closest
> value and all decoders would take the aspect in consideration no
> matter what it is.. This means that the PAL / NTSC values are just
> examples.

Well, the table I have reported at the beginning is taken from the
standard, and other encoder (mpeg2enc) uses 3 as aspect for PAL 16/9.

It's a matter of being uniform with the common practice.

> 
> In short the encoder should not try to second guess what the user want.
> 
> You had probably missed something. e.g.
> 
> now you have (352/288)/(16/9) = 0.6875
> if you had      (360/288)/(16/9) = 0.703125
> 
> You see in the first case the closest match is #2 = 0.6735
> (#2 delta=0.0140 vs.  #3 delta=0.0156)
> in the second case it is nearly perfect match #3 = 0.7031
> (delta=0.000025)
> 
> 
> (I excuse in advance if I've done some stupid calculation mistake ;)

You calculation are correct. And I also sayd in my mail that the code
in the for cicle is correct, aspect 2 is the one that gives the closest
match for PAL 16/9, just the standard also gives 3 as aspect for PAL 16/9.

Giacomo