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

[Reimar Döffinger]

Hello,
On Wed, Aug 02, 2006 at 04:01:59PM +0200, Baptiste Coudurier wrote:
> > Yes, but I like to it more explicit. But the "more correct" is more
> > about signed vs. unsigned.
> 
> I prefer the simplest way.

Which I usually find is unsigned, since that simplifies many checks
(e.g. for malloc) from "i < 0 || i > max" to just "i > max".


> > Well, there could be a length array for tag type, but how to handle
> > MXF_TT_FUNC or MXF_TT_UID_ARRAY?
> 
> hummm MXFStrongRefArray is 8 + 16n, I find that MXF_TT_FUNC weird.
> Maybe both treated as exceptions.

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?

> > True, but except for the extra checks (which if you insist I can remove)
> > it is exactly as ugly as the current code. But only once instead of how
> > many? five times?
> 
> I do not agree here. Code is duplicated, right, but clear, simple and
> obvious. IMHO if it's about factorization, factorize everything that can
> be but without loosing simplicity.

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

> > Maybe, but if the end result will be really better? And what if one day
> > we wanted to add checks if maybe tags are used that the specs do not
> > allow in that section?
> 
> That would mean non valid files. Specs are specs and SMPTE usually
> really consider everything in the specs. Like I said, if we really want
> to support broken files, that code needs to be really isolated. Your
> file with '0' tag size is broken.

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.

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

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

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

Greetings,
Reimar D?ffinger