[FFmpeg-devel] Process (Was: [PATCH][7/8] Add VA API accelerated H.264 decoding (take 4))

[Michael Niedermayer]

On Tue, Feb 10, 2009 at 11:18:08AM +0100, Gwenole Beauchesne wrote:
> Hi,
> 
> On Mon, 9 Feb 2009, Michael Niedermayer wrote:
> 
> > And above all, do you consider my suggestions bad? If so that should be
> > discussed, maybe i miss something and you have a better idea for the
> > optimal design but if not, then where is the problem? we arent disagreeing
> 
> Yes, your factorisation model is a little suboptimal.

I didnt really ask if you like my criteria for accepting patches, obviously
you do not because your patch doesnt meet it without some additional work.

What my question was, was if you agree about my technical suggestions to
improve your patch or if you had some better ideas.


> I don't object 
> factorisation, but I can only see two reasonnable entry-points for it.
> 
> 1) At the very begining. However, you have to have an overview of other 
> work, otherwise you would tend into an 

> over-engineered academic work. If 

yes thats a good summary, we in general dont have enough knowledge of 
everything so that we could factorize code before it existed.


[...]
> 2) Third iteration, i.e. third point of view. Either a 3rd party factoring 
> code from 2 concurrent implementations or one of the original author 
> willing to add support for a 3rd spec. A third point of view is needed to 

there is no 3rd party, and neither you nor nvidia will do the factorisation
once their code is in. Stephen warren said he doesnt have the time IIRC and
its rather obvious you wont either.

So my only choice to avoid code duplication is to require it to be done
before support for any further API is added.
if your patch would have been submitted a month before nvidias then they
now would likely be in your position.


[...]
> >> At the very least, you should have commented on the real implementation
> >> earlier/beforehand, not many weeks after commenting on pure *cosmetics*.
> >> That's a terrible waste of time, and totally suboptimal process. IOW,
> >> cosmetics are the last things to care about in a process, especially if
> >> those are things that could be done through an automated tool.
> >
> > Why did you not realize the problems immedeatly?
> 
> I did, but as any newcomer I think, we assume existing practise (i.e. 
> current code already committed) was the right one and don't want to bother 
> others with what they would think are "pointless discussions" or trivial 
> things to check from the code.
> 
> So we (well at least me, I don't know for others) then base their work off 
> current practise, make the effort to read existing code. Isn't what you'd 
> expect?

I expect patches to be written to the best of the authors knowledge and
abilities and to follow the well stated "cosmetic" rules. but later is
not much work in general to fix if the first is followed.


> Otherwise, I already saw reactions like: "the code has your 
> answers, if you bother" ; "if you can't read any code, you are a beginner 
> programmer", etc. That's also has a psychological effect, some people like 
> bitching at others easily and newcomers would like to avoid that. IOW, in 
> order to keep one's mental health sane, it's better not attempt things 
> that would get those usual reactions. => base your code off current 
> practise.
> 

> Now, you could also warn people that: "be careful, FFmpeg code is probably 
> architecturally broken, don't assume anything! Come and discuss first, we 

what may be acceptable as interface to one API may not be for more than 1
besides you didnt expect a program of the size of ffmpeg to be without bugs
and suboptimal parts did you?


> do promise we won't bite you. If we do, OK, if we would accept your patch 
> wihout compromise. ;-)".
> 

> > if so why do you belive i knew more when even the author of the patch 
> > apparently didnt notice every problem.
> 
> Because you are actually the one who know more the surrounding code and 
> has the best overview of the code. So, you are a reference, if you 
> accepted a patch, then it was a correct practise and incentive to others 
> to operate the same.

humans make mistakes, we should learn out of them not not repeat them
you ask me to repeat my mistake and accept your patch with the same issues
as previous ones.


> 
> Moreover, if you don't change the current code either for 
> weeks/months/years, it means it's either the most correct approach to 
> follow, or the code is totally obsolete and should be marked as such. i.e. 
> document it as "bad practise, please don't follow this model, needs to be 
> changed, etc." + one or two references to list archives.

or i am limited to work just 24h a day and cant do everything i might want.


[...]
> 
> >>> you can maybe gain some advantage for VAAPI over VDPAU ;)
> >>
> >> You know, VA API already has a clear advantage over VDPAU: it supports all
> >> three major APIs and implementations. ;-) So, I'd even say, if you want to
> >> have a common FFmpeg base struct, you'd better deriving it from VA API as
> >> it's descriptive enough to suit your purposes.
> >
> > maybe and maybe iam not opposed but then this sounds all to familiar ...
> > SDL supports it all X11,XV, ... ffplay supports just SDL so it has to be
> > great ...
> > seriously i wish ffplay had native XV & X11 support _instead_ of SDL
> >
> > thus the idea of just supporting VAAPI has to be considered carefully
> 
> It's not the point, it would be for approach (A) if you wanted common data 
> structures directly at the FFmpeg level then have converters to the target 
> APIs. In that case, VA API would indeed have all the necessary stuff 
> already (but the MPEG-2 IDCT-level acceleration, point granted). So, it 
> would have been enough grab/rename/whatever the data structures, in that 
> case.

i review patches and you know you didnt submit a patch with renamed VAAPI
data structs. (thats not to say that this approach would be good nor that it
would be bad ...)
So stop the hypocrisy and asking me who doesnt have the time, hardware nor
fine knowlede of VAAPI to do what you, who apparently had all that didnt
manage to do.

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- 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-devel/attachments/20090210/c7ffbbaf/attachment.pgp>