[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

[Måns Rullgård]

Roman Shaposhnik <rvs at sun.com> writes:

>> 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. 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.

Since FifoBuffer is defined in avformat.h, having a couple of
functions for fast access next to the definition makes sense.  If the
definition changes, any shared library user is screwed anyway.
Besides, we have library version numbers to protect against that.

If we want the possibility to change structs without breaking binary
compatibility, we have to remove the definitions from public headers
entirely.

> 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.

I've seen it stated somewhere, but it was probably only in an email to
the list.  Official policy should be written in a file in svn.  An
email is easily overlooked.

> [bitching from both sides]

Please, be civil and don't let this turn into a flame war.  I'm sure
nobody has any bad intentions.

> 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.

That seems like a good explanation to me.  Now, I agree with Michael
that stating the reason for checksum changes in the commit message is
good.  It simply removes any doubts.

-- 
M?ns Rullg?rd
mru at inprovide.com