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

[Baptiste Coudurier]

Michael Niedermayer wrote:
> 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?

Im not aware of any problem with progressive 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

Why don't you send patches to common parts, which I will know will
introduce other bugs ? So you can experience that "rejected" feeling.

You broke ./configure --enable-debug with your h264 bugs without asking
anyone and without even willing to fix, and you just said "send patch",
that proves how you care about others.

I don't know any file with different height in container than sum of
both field, since mjpeg supports any resolution, while some codecs don't
, and that's why mov do that.

> the proper solution is to use the sum of the height of both fields not the
> container height

Again feel free to implement that solution, you are mjpeg.c maintainer,
and I bet that bug won't be fixed anytime soon.

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

You can understand h264 complicated code, but not what a 10 lines patch
does ? Nonsense, you do understand what the code does, you are just
unhappy with it, and you just complain, and hope that by complaining
patch sender will give up.

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

You consider them corner cases, because you don't have those files at
hand. They aren't corner case, bottom field first is used with every
NTSC content, and I got like 20 files with odd field height.

Now even rare corner cases are not a reason to not fix the bugs.

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

And I think you really bad treated those SoC students, requesting for a
simple cosmetic patch after you applied the patch, while you could do it
yourself is clearly abusing. Laziness have become the main characteric
around here.

He is unfairly treated, and you should know better who is unhappy and
who is just not speaking, but I know you don't care about team spirit.

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

I don't think so, and I can't wait the end of your laziness to fix a bug.

> 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

We will see, and here you are being aggressive, also you should fix non
compliant code in ffmpeg before talking, and there are some in mpeg12
encoding about buffer size values, you are not aware of it, sadly for
you, and I even already fixed some non spec compliant behaviour. Please
really reconsider yourself, and admit that you are not aware of
everything, Im not either.

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

If you don't know specs, you are not very well placed to maintain the
code IMHO.

1 line and adding a comment in doxygen to mention the value of
interlace_polarity. I don't consider that cosmetics.

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

You did not ask for a description of the problem. You answered
agressively and reread what you answered in the first time please, you
are just lying here, and you tend to lie more and more in that thread.

I do accept some bugreports when it's about code I know in a reasonable
way, and there is no need to pester anymore the guy to send a bugreport
because of your own laziness.

Because Im a MAINTAINER, and because Im being active, it's all about
meritocracy.

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

Diego already answered to that.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312