[Ffmpeg-devel] Re: [Ffmpeg-cvslog] r6161 - in trunk: libavcodec/dv.c libavcodec/dvdata.h libavformat/avformat.h libavformat/dv.c tests/ffmpeg.regression.ref tests/libav.regression.ref tests/rotozoom.regression.ref

[Michael Niedermayer]

Hi

On Mon, Sep 04, 2006 at 06:27:29PM -0700, Roman Shaposhnik wrote:
> Hi
> 
> First of all -- is it really a good practice to follow-up
> on fmpeg-cvslog instead of ffmpeg-devel ? I don't like
> littering hence my reply to -devel.

indeed, -devel seems more appropriate, maybe we should even add a general
suggestion that disscussions about commits should be on -devel to the docs
...


[...] 
> > avformat.h is a public header, changes to it should be disscussed on
> > ffmpeg-dev (even more so as you are not the maintainer of it)
> > such static inline functions are also not a good idea in a public header
> > because they make the user app unnecesarily depend on the implementation
> > and thus lead to binary compatibility issues, furthermore these functions
> > are used just by the dv code so they really dont belong to avformat.h
> > and does the usage of these functions even merit that they are static inline?
>  
>   I would expect fifo_peek to benefit *a lot* by being a static inline,
> since its being used in a pretty hot loop. 

i would expect it to benefit more if the % wherent there, and calling it
twice for x and x+1 also seems not optimal at all, maybe a fifo_peek()
which always returns 4 byte in an big endian int would be more generically
useable and faster for the specific case in dv


> fifo_drain surely can be 
> a plain function without losing much performance. What would you like
> me to do with these functions ? If I move them out of avformat.h
> completely (that is -- there won't be even declararions of them) then
> next time somebody changes internal implementation of the FifoBuffer
> my private versions of fifo_peek and fifo_drain will be screwed -- not
> a good idea as far as I can tell. Please advise.

IMO move all the fifo specific stuff (together with the 2 new functions)
to its own header, maybe fifo.h, none of this belongs to avformat.h,
maybe we could one day move it to avutil but thats a seperate issue, it
definitly doesnt belong to the main public header of libavformat though


> 
> I understand your your comment about making changes to the portions of
> the ffmpeg which are maintained by others, yet, I haven't seen such a
> policy explicitly stated anywhere. Thanks for making me a poster child
> for stating the policy. 

http://ffmpeg.mplayerhq.hu/ffmpeg-doc.html#SEC36
---
5. Do not commit changes to the build system (Makefiles, configure script) which change behavior, defaults etc, without asking first. The same applies to compiler warning fixes, trivial looking fixes and to code maintained by other developers. We usually have a reason for doing things the way we do. Send your changes as patches to the ffmpeg-devel mailing list, and if the code maintainers say OK, you may commit. This does not apply to files you wrote and/or maintain
---
9. Do NOT commit to code actively maintained by others without permission. Send a patch to ffmpeg-devel instead. If noone answers within a reasonable timeframe (12h for build failures and security fixes, 3 days small changes, 1 week for big patches) then commit your patch if you think it's OK. Also note, the maintainer can simply ask for more time to review!
---


> btw, I've got a question: do I have a right to
> scream and shout next time somebody makes a [trivial] change to dv.c ? 

of course you do


> 
> > additionally, expect to loose your svn write account if you add such stuff
> > to non dv specific code the next time without discussion on ffmpeg-dev.
> > you have done this several times by now ...
> 
>   Michael, this is not nice. First of all -- please tell me exactly
> when was the last time I done so ? Please be specific -- otherwise
> you sound like a bickering asshole. 

i honestly dont know when was the last time, but one case i
could find was the change to common.h (now bitstream.h) at 
r2874 Fri Mar 12 23:39:38 2004 UTC and
r2892 Sun Mar 14 22:09:58 2004 UTC
thats indeed a little long ago, i thought it was not that long ago so
maybe ive overreacted but still, developers should not randomly change
code maintained by others, it simply leads to bugs and chaos especially
as the number of developers increases


> Don't get me wrong though -- I do
> understand all the technical points you've made. I do actually
> agree with most of them but what I don't agree with is the attitude
> you've demonstrated towards my person on a number of occasions now.
> And what is especially irritating is the fact that you're not even
> capable of simply saying that you don't like me (or whatever) and
> you don't want me on this project. If I irritate you so much -- just
> go ahead and say so. 

i can assure you that i hate everyone equally and that i have and had 
nothing against you personally or anything like that, its just that if
someone, be it you or someone else changes some code which
isnt his _AND_ i disagree with the change then ill tend to get quite upset
and flame him, wouldnt you get upset too if i do some changes to dv.c which
you strongly dissagree with? the solution is simply to disscuss changes on
ffmpeg-devel before doing them
and furthermore fact is i was pretty tired when i wrote my reply to you
which probably didnt improve my politeness level either, in real world
people would have noticed that i was tired. on a mailinglist things like
that arent noticed which causes further missunderstandings ...


[...]
> > could you explain why the regression checksums change, your commit message
> > makes the change sound like a pure restructuring which wouldnt change the
> > output?
> 
>   Michael, do you really want to know or are you just fsck'ing up with
> me trying to see whether I was stupid enough to commit a change without
> understanding why it changed the regression checksums ?I can not really
> tell the difference, you know. 

of course i really wanted to know why the checksums changed, IMHO every
change to the regression checksums should be documented on svn log


> 
>   Anyway, here's the reason: There was a tiny bug that led to every DV
> stream we've generated having the first two frames labeled as 
> "frame 0". Now the second frame gets properly tagged as "frame 1".
> That's the only difference visible to the regression testing. Not
> visible to the regression testing are things like aspect ratio now
> being recorded properly.

ok


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is