[FFmpeg-cvslog] r20938 - trunk/libavcodec/h263.c

[Diego Biurrun]

On Tue, Jan 05, 2010 at 03:20:47AM +0100, Michael Niedermayer wrote:
> On Mon, Jan 04, 2010 at 08:23:23PM +0100, Diego Biurrun wrote:
> > On Mon, Jan 04, 2010 at 05:15:37PM +0100, Michael Niedermayer wrote:
> > > On Mon, Jan 04, 2010 at 04:51:01PM +0100, Diego Biurrun wrote:
> > > > On Mon, Jan 04, 2010 at 04:46:29PM +0100, Michael Niedermayer wrote:
> > > > > On Mon, Jan 04, 2010 at 04:39:07PM +0100, Diego Biurrun wrote:
> > > > > > On Mon, Jan 04, 2010 at 04:24:37PM +0100, Michael Niedermayer wrote:
> > > > > > > On Fri, Jan 01, 2010 at 09:43:35PM +0100, Michael Niedermayer wrote:
> > > > > > > > On Sun, Dec 27, 2009 at 03:32:23PM +0100, diego wrote:
> > > > > > > > > 
> > > > > > > > > Log:
> > > > > > > > > Remove commented-out debug console output.
> > > > > > > > [...]
> > > > > > > > > @@ -2642,7 +2636,6 @@ void mpeg4_pred_ac(MpegEncContext * s, D
> > > > > > > > >  static inline void mpeg4_encode_dc(PutBitContext * s, int level, int n)
> > > > > > > > >  {
> > > > > > > > >  #if 1
> > > > > > > > > -//    if(level<-255 || level>255) printf("dc overflow\n");
> > > > > > > > >      level+=256;
> > > > > > > > >      if (n < 4) {
> > > > > > > > >          /* luminance */
> > > > > > > > [...]
> > > > > > > > > @@ -5807,7 +5771,6 @@ no_cplx_est:
> > > > > > > > >              if(   h_sampling_factor_n==0 || h_sampling_factor_m==0
> > > > > > > > >                 || v_sampling_factor_n==0 || v_sampling_factor_m==0){
> > > > > > > > >  
> > > > > > > > > -//                fprintf(stderr, "illegal scalability header (VERY broken encoder), trying to workaround\n");
> > > > > > > > >                  s->scalability=0;
> > > > > > > > >  
> > > > > > > > >                  *gb= bak;
> > > > > > > > [...]
> > > > > > > > >          s->time_base+= time_incr;
> > > > > > > > >          s->time= s->time_base*s->avctx->time_base.den + time_increment;
> > > > > > > > >          if(s->workaround_bugs&FF_BUG_UMP4){
> > > > > > > > >              if(s->time < s->last_non_b_time){
> > > > > > > > > -//                fprintf(stderr, "header is not mpeg4 compatible, broken encoder, trying to workaround\n");
> > > > > > > > >                  s->time_base++;
> > > > > > > > >                  s->time+= s->avctx->time_base.den;
> > > > > > > > >              }
> > > > > > > > > @@ -5936,7 +5895,6 @@ static int decode_vop_header(MpegEncCont
> > > > > > > > >          s->time= (s->last_time_base + time_incr)*s->avctx->time_base.den + time_increment;
> > > > > > > > >          s->pb_time= s->pp_time - (s->last_non_b_time - s->time);
> > > > > > > > >          if(s->pp_time <=s->pb_time || s->pp_time <= s->pp_time - s->pb_time || s->pp_time<=0){
> > > > > > > > > -//            printf("messed up order, maybe after seeking? skipping current b frame\n");
> > > > > > > > >              return FRAME_SKIPPED;
> > > > > > > > >          }
> > > > > > > > >          ff_mpeg4_init_direct_mv(s);
> > > > > > > > 
> > > > > > > > removing above lines makes the code harder to understand
> > > > > > > 
> > > > > > > also, just to clarify, i think this should be reverted
> > > > > > 
> > > > > > How about adding the information as comments instead of as crufty debug
> > > > > > statements.
> > > > > 
> > > > > Thats possible but more work than just reverting these hunks
> > > > > (and iam not volunteering to do that nor would i ask someone else to
> > > > >  do such cosmetic work)
> > > > 
> > > > Hardly, this is less than the complete commit, it boils down to cutting
> > > > and pasting or modifying a diff by hand...
> > > 
> > > I do volunteer to revert these hunks if you are to busy to do it, i dont
> > > volunteer to convert them to comments and check them against the code to
> > > make sure that they are still meaningfull as comments. One of them was
> > > a if() printf() that alone is going to cost more time than the 20sec
> > > copy & paste into patch && svn di && svn ci
> > 
> > How hard can it be - here is the patch...
> 
> id say too hard, but lets see, there where 4 hunks, your patch contains 3.
> Something has been lost ...

I couldn't come up with a good replacement right away, but

  dc must be in the [-255,255] range

might work.

> i still think copy and pasting the 4 hunks into patch is going to be the
> quicker way to fix this, thats not disputing that you eventually will come
> up with a convertion to proprer comments just that the work seems out of
> proportion to the purely cosmetic gain

What is out of proportion is the ensuing discussion. What is
disheartening is the lack of motivation to actually fix the current
mess. And what is less work is just committing my changes. Done.

The problem I see is that this touches upon a deeper issue: We have some
very complex code parts that work, but are crufty and impenetrable.  The
result is that nobody but you dares work on them and the fools that try
anyway receive nasty surprises.  In the end people get frustrated for
wasting their time and you get frustrated because nobody lends a helping
hand.  Does "H.264" ring a bell?

Diego