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

[Baptiste Coudurier]

Michael Niedermayer wrote:
> Hi
> 
> On Wed, Apr 11, 2007 at 02:17:14PM +0200, Baptiste Coudurier wrote:
>> Hi
>>
>> 3 patches:
>>
>> - remove useless MpegEncContext.
>> - fix odd field height decoding.
>> - fix some interlaced content.
> 
> what is some?

http://samples.mplayerhq.hu/V-codecs/MJPEGs/angels_480-mjpegcompress.avi

and any of bottom field first in avi.

>> justification:
>> odd_heigth.mov, this sample is 720x405, 
> 
> where is this sample?

mplayerhq, where do you want it to be ?

>> 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, anyway consider that as
a bug report and feel free to fix it the way you want.

> what the 3rd patch does is still unclear to me ...
> 
> 
>> ODML specs specify the use of AVI1 tag, and meaning of polarity (0 for
> 
> what does ODML have to do with mjpeg? is this series of patches related to
> mjpeg in avi? the sample file whos location we dont knoe is a .mov not avi

RTFSpecs. ODML (MJPEG DIB)

>> prog, 
> 
> prog? you mean progressive, if so write progressive

why ?

>> 1 means encoded from first field, 2 means encoded from second
>> field), 
> 
> so you speak about a variable somewhere in the ODML-avi container spec
> which stores a progressive/interlacaed and temporal field order or is it
> rather spatial order flag? hows that related to mjpeg and how to the patches?

RTFSpecs again.

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

> sorry but if you cannot clearly describe what your patches do and why then
> they are rejected
> the amount of time i spend with your patches likely exceed he time you spended
> writing them, i feel like iam being throwen random hacks at and then you even
> pester me so i waste even more time looking at the mess again to somehow
> make sense of what exactly the patch really changes while you know exactly
> but dont describe it in a understandable way

Well, you'll just get bug reports for now, if this will save you time,
as you wish.

Btw, quoting ffmpeg-doc.texi:
"All patches posted to ffmpeg-devel will be reviewed, unless they
contain a clear note that the patch is not for SVN."

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

"Also please do not submit patches which contain several unrelated
changes. Split them into individual self-contained patches; this makes
reviewing them much easier."

Are you having problem tracking patches/bugs ?
Where is roundup bug tracker ?

Btw, Im still asking for full svn dump.

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