[FFmpeg-cvslog] r20938 - trunk/libavcodec/h263.c
Michael Niedermayer
michaelni
Tue Jan 5 20:43:51 CET 2010
On Tue, Jan 05, 2010 at 11:29:46AM +0100, Diego Biurrun wrote:
> 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.
hmm, i dont know, its not checking dc ...
>
> > 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.
My fix is to revert this. You refuse to do that. I dont really mind
an alternative fix but i cannot fix what you precive as mess because
i do not precive these //printf("some error condition") as mess.
To me a note on toilet paper is as good as one on gold framed under glass
> 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
I think this is a quite skewed view of the truth.
First the underlaying algorithms are complex, this isnt a "hello world"
program, to understand the code an understanding of C and MC-IDCT-VLC
coding is required. That is so no matter what is changed on the code.
Second as you say yourself the code works -> thus people have little
motivation/reason to work on it.
> 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.
To be honest and blunt, i dont think the changes you will do to this
code will improve its readability. Thus i feel my time is wasted if
i did spend time on this.
But id be honestly happy to be proofen wrong, for example if you instead
of doing (IMO silly) cosmetic changes between //printf(" and //
would add new doxygen comments to document what the individual functions
do and maybe there are even case where code could be factored or changed
to be easier to read but i think pure doxy alone would already be usefull
then iam very much willing to help.
But as said i plain refuse to spend time on thinking how a
// if() printf("error condition") could be reformulated in nice english
Theres so much that could be done
examples:
the #if 1/0 #else code: Instead of removing one side, one could test if
it still works and if it does document it like:
+ // An alternative, simpler and easier to understand implementation
+ // that has been disabled because it is slower but left under #if 0
#if 0
If they are removed its a step back in terms of understandability ...
Documenting functions
Documenting variables or changing their name where its bad
Writing some high level overview of where the entry points are and how
the different functions in general interact
all that and more id be very happy to see done, but purely cosmetic changes
feel like a waste of time to me
> Does "H.264" ring a bell?
H.264 is an entirely different beast, its much more complex than h.263
and even i as author tend to need to read spec & code often. But to a large
extend the same commnents apply, h264 itself is very complex and quite
messy, any implementation that performs at reasonable speed will be
messy & complex. Improvments to our implementation are welcome of course
but the h263 code should be easier ...
PS: about the "nasty surprises" to people who change code. Its obviously
alot better to flame than to leave non ideal changes pass, thats one
of the things why having a maintainer who checks changes is a good
thing ... prevent worsening of the code be that in form of bugs or
readability. This also was a quite central issue of mplayers decay,
poor changes done by people who did not understand the (very messy but
working code) and then turned it into something not so good working.
The next step then was the claims that its all wrong and needs rewrites.
Summary: Well working code that noone understood + changes -> buggy code
that (now obviously) noone understood.
I dont want that to happen to libavcodec ...
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20100105/753feb1f/attachment.pgp>
More information about the ffmpeg-cvslog
mailing list