[Ffmpeg-devel] [PATCH] mjpeg cleanup and again interlaced fix

Sat Apr 14 11:57:22 CEST 2007

Hi

On Sat, Apr 14, 2007 at 03:31:15AM +0200, Baptiste Coudurier wrote:
> Michael Niedermayer wrote:
> > Hi
> > 
> > On Sat, Apr 14, 2007 at 01:50:50AM +0200, Baptiste Coudurier wrote:
> >> Michael Niedermayer wrote:
> >>> Hi
> >>>
> >>> On Sat, Apr 14, 2007 at 12:38:48AM +0200, Baptiste Coudurier wrote:
> >>> [...]
> >>>>>> and setting height *= 2 is then
> >>>>>> wrong, correct is to set it to container height.
> >>>>>> Since height change every field, do that for every field, not only for
> >>>>>> first picture.
> >>>>> what the 2nd patch seems really to do is 
> >>>>> 1. correct a bug where half the init code was only exceuted for width/height
> >>>>> changes during the first frame, this worked fine with even interlaced height
> >>>>> but with odd interlaced height the height changes after every field and so
> >>>>> the init code gets executed after every field but due to the bug just half
> >>>>> of it gets executed ...
> >>>>> 2. replace frame height= jpeg height*2  by frame height= container height
> >>>>> why this is hidden under a million of conditionals is unclear
> >>>>> also its unclear what will happen with mov style w/h missuse for croping
> >>>> You are maintainer of mjpeg.c, you should know, 
> >>> i should know the effects of YOUR patch?
> >> you should know "why this is hidden under a million of conditionals is
> >> unclear",
> > 
> > currently the code multiplies height by 2 that has to be hidden because it
> > well is wrong for non interlaced content, but setting height to the container
> > height like after your patch is a different story, why this is still under
> > the set of if() for interlaced content is not clear and thats your code not
> > mine ...
> 
> It's not my code, I kept original check, and only removed first_picture
> and I explained why, I'll repeat in hope to be more clear, height change
> for every field with that odd height sample.

well you set height to container height, and my question still is why only
in the interlaced case?

> 
> >> you also should know what will happen with mov style w/h CORRECT cropping,
> >> aren't you also mov.c maintainer ?
> > 
> > well i think mjpeg after the 2nd patch from you will break with mov
> > cropping, which would mean your 2nd patch is buggy
> 
> Well, it might break for cropping, but it is IMHO less buggy than
> before, patch addresses an issue, and does not address the other issue,
> but still height *2 is wrong.

i dont know if its written in any docs but we have always rejected bugfixes
which knowingly introduce other bugs
the proper solution is to use the sum of the height of both fields not the
container height

> 
> >>>> anyway consider that as
> >>>> a bug report 
> >>> ok, then write a proper bugreport
> >> You are aware of the problem now, you have the sample.
> > 
> > iam a volunteer, if people dont provide proper bugreports i wont look at
> > the bug, and giving me a sample and a patch i dont understand is not
> > a bugreport
> 
> hey michael Im also a volunteer, I consider trying to fix the bug myself
> to avoid you looking at it better than simply bugreporting, now I don't
> understand why you always answer with aggressivity and without any trust

i do not intentionally awnser agressively and review is not about trust
a patch which i dont understand i wont approve

[...]

> >> but still a problem
> >> for you.
> > 
> > no thats false
> 
> Seriously you don't consider having bugs in ffmpeg a problem for you ?

i dont consider it a problem for me personally, sure i prefer fewer bugs
but not at the cost of code i dont understand
i consider clean, understandable, maintainable and simple code more important
then missing support for some rare corner cases

[...]
> 
> > i think you know what you patch does and why and its not unreasonable to
> > expect you to explain this to me instead of asking me to go over a few 100
> > pages of manuals ...
> 
> Of course I know, but Im quite exhausted feeling no trust whatsoever and
> always nitpicking, and I think Im not the only one developer feeling
> that.

i think steve lhomme feels similarely, at least he did in the past maybe 
you want to fork ffmpeg together with him
here code must pass review to reach svn, no matter how much
the author complains and feel unfairly treated, even if its true and he
is unfairly treated which i dont think so, just look at the SOC students
as example, they have resubmitted their patches many many times and didnt
complain, you submit the code once, pester me if i dont review it, ask me
to read a large manual instead of awnsering a question and you didnt
resubmit the patch without cosmetics at all

also i think you pick hard areas of code to work on which is a reason
why alot of code from you is rejected

the mpeg-ts muxer you mentor for SOC with xiaohui as student also is a
hard area
if it doesnt share its core with the existing mpeg-ps muxer or isnt
fully mpeg-ts spec complaint it will be rejected and with little disscussion
iam saying that already now so you wont be surprised, we already have a
broken mpeg-ts muxer in svn we dont need a second differently broken one
the same is of course true for the other SOC projects but i think they
have a better chance to succeed, you picked with mpeg-ts and xiaohui
the IMHO least likely successfull project

> If you honestly think Im throwing garbage at you, then Im sorry
> because you have no idea about how much time I spend verifying cosmetics
> with svn diff, and taking care of explaining, now you never understand,
> what can I do ? Spend more time ? Well Im also doing a lot of things.

i dont know how much time you spend looking for cosmetics, what i know is
there are still cosmetics in the 3rd patch, also i dont know how much time
you spend explaining, fact is i dont understand the 3rd patch with the
comments, and you did point to the ODML spec and complain about my questions
being agressive instead of awsnering them

> 
> >>>>>> and only set polarity when first field is being decoded, use of
> >>>>>> new second_field in context.
> >>>>> this makes no sense, iam i supposed to read the jpeg and odml specs and then
> >>>>> reverse engeneer what the patch does? your comments are as usefull as 
> >>>>> /dev/random
> >>>> Up to you, fix bugs in the code you are maintaining if you don't want to
> >>>> review patches.
> >>> iam not aware of any proper bugreport 
> >> You are just lying.
> > 
> > no its the truth that iam not aware of any proper bugreport related to this
> > if you are aware of one why dont you provide a link?
> > also you cant expect me to remember every bugreport ...
> 
> I consider my mail as a bugreport, you are asking me to spend more time
> to something you are now aware about, you can just mark the mail as
> important/to fix and then come back later. If I do some efforts to
> please you, I think I deserve some effort in return, Im putting a lot of
> myself into ffmpeg too.

iam just asking for a description of the problem, what happens? segfault
video with serious artifacts? video with a line too much, video with a
line too little? we dont accept bugreports without such basic information
why do you think you should be treated differently than everyone else?

[...]
> >>>>> its not even clear which of your comments relates to which of the patches
> >>>>> that is without looking at the patches ...
> >>>>>
> >>>>> also the 3rd patch contains cosmetic changes!
> >>>>>
> >>>>> and each patch should be in a seperate mail
> >>>> Where is it mentioned ?
> >>> maybe it isnt in the docs, i will change this, thanks for noticing
> >> Changing policy at your own will without asking other
> >> reviewers/developpers ? Is that democracy or tyranny ?
> > 
> > you are free to start a vote, i will revert the change if the majority
> > wants it
> 
> Honestly problem is that you too oftenly do that, and people is not
> caring at all, look at how many people answer or discuss about sensitive
> topics, so people might think but not say because they don't want to
> fight/flame.

this is ffmpeg-dev people do flame if they disagree you must be blind if
you cant see that
if noone complains then noone disagrees thats pretty much the definition of
disgagree

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

Democracy is the form of government in which you can choose your dictator
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070414/afcff1ff/attachment.pgp>