[Ffmpeg-devel] Re: [PATCH] MXF tag parser

[Baptiste Coudurier]

Hi

Reimar D?ffinger wrote:
> [...]
> 
> It is currently only used for mxf_read_metadata_pixel_layout. The idea
> was mostly not to bloat the main switch with large code parts that in
> the end will only be used in one place.
> Of course all this is only a proposal, so before I waste any time with
> this: do you see a realistic chance of applying most of it? If not, are
> there parts that have that chance? Or what approach would you suggest to
> reduce duplicate code? Just a separate function to parse the UID arrays
> as a first step?
> 

I'm sorry if you have the feeling of wasting your time, that's really
not what I want.
Definitely factorizing tag reading is needed, and your approach is good.

I would like to keep naming scheme homogenous and simple. Naming
convention atm are based on specs names. For having worked much in mov.c
 and movenc.c, that's the most annoying thing.

> [...]
> 
> That code duplication makes it really a pain to extend and maintain
> IMHO, at least when there is no maintainer. How do you know if "the same" code
> is really the same or if it is a minor bit different that you simply
> missed?

Code follows specs logic and really follows engineering guidelines and
philsophy, also with variables names. Someone looking at specs will
definitely understand the code.

> But regardless, what exactly in that code part made it loose
> simplicity? The only thing I changed was adding three additional ifs,
> the rest is copy-and-paste, so if I just remove those it is clear and
> simple again? I'm fine with that, but as you already said, I think we have
> significantly different standards for clean and simple...

I personnally find casts pretty obfuscating.

> [...]
> 
> 1) I admit coming from MPlayer my goal is in the end to support as many
> files as possible, broken or not.
> 2) What I actually was thinking of was checking a bit the
> standards-compliance of files and printing a warning if they are not.
> Which would include warning of tags if they are in some place they don't
> belong to.

Well, I understand. Problem is that MXF is already bloated and
implemented wrongly, look at AVID, SONY XDCAM, broadcast servers flavours.
I really would prefer it very strict.
Checking sizes for each tag, is IMHO too much, checking for 0 size is ok.

>>> I like this approach because it is quite flexible.
>> Generic parsing will be even more flexible: just add tag and where to
>> store based on type of object. MXF is using "object oriented" approach.
> 
> Hm. I probably misunderstood what you were suggesting above.

Thing is if tags reading is factorized then I see no point why not
factoring all functions into one, since they do basically allocating
struct and reading tags.

>> I don't, but that's my own point of you, don't take it badly, like I
>> find GXF demuxer code really obfuscated.
> 
> At least I have more comments :-P. I am interested in suggestions for
> improvements, it always has the possibility that there is a solution we
> both find better *g*

Hehe. I'll look at it.

>>> Of course another possibility would be to keep the switch-cases as they
>>> currently are and just add functions to read those UID-arrays. That
>>> alone would be quite a progress IMHO.
>> That might be good, yes:
>>
>> case 0x....: mxf_read_strongref_array(pb, &refs_array); break;
> 
> Well, I guess it would at least need a count argument or so, too.
> Oh, and I really think your last patch removed one of the code
> places that I found most annoying, especially with those
> extremely-overlong lines *g*

Yes maybe. Maybe something like: { tag, type, reading_function }

Ah thanks, don't worry Im really aware that code duplication is bad, I
just prefer duplication over obfuscation, and that's true I like using
empty lines to separate blocks and using functions with specific names.
I might be picky..

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