[FFmpeg-devel] [PATCH] mxfdec: make it work with other calling conventions

[Reimar Döffinger]

On Tue, Jun 29, 2010 at 01:02:05PM -0700, Baptiste Coudurier wrote:
> On 6/29/10 11:51 AM, Reimar D?ffinger wrote:
> >On Tue, Jun 29, 2010 at 08:30:01PM +0200, Reimar D?ffinger wrote:
> >>Hello,
> >>currently mxfdec assumes that it can unpunished pass more arguments to functions
> >>than they were declared with.
> >>This is not true in general, in particular not for stdcall.
> >>While I am not aware of any FFmpeg platform using it, IMHO this code is still
> >>wrong.
> >>Attached is a patch that fixes it, and I think it is not particularly bad.
> >>It also fixes the last remaining warnings ("function declaration is not a prototype")
> >>in that file.
> 
> I don't have this warning.
> gcc version 4.4.4 (Ubuntu 4.4.4-6ubuntu2)

Right, I had been using -Wstrict-prototypes

> >+#define METADATA_READ_FUNC(name) int name(void *arg, ByteIOContext *pb, int tag, int size, UID uid)
> 
> I don't like the #define.

Well, the alternative is having to change every single read function
manually in case you'd ever need an additional parameter.
And I found it very hard to find them.

> >@@ -919,11 +931,14 @@
> >              url_fseek(s->pb, klv.offset, SEEK_SET);
> >              break;
> >          }
> >+        if (IS_KLV_KEY(klv.key, mxf_primer_pack_key)) {
> >+            mxf_read_primer_pack(mxf);
> >+            continue;
> >+        }
> >
> >          for (metadata = mxf_metadata_read_table; metadata->read; metadata++) {
> >              if (IS_KLV_KEY(klv.key, metadata->key)) {
> >-                int (*read)() = klv.key[5] == 0x53 ? mxf_read_local_tags : metadata->read;
> >-                if (read(mxf,&klv, metadata->read, metadata->ctx_size, metadata->type)<  0) {
> >+                if (mxf_read_local_tags(mxf,&klv, metadata->read, metadata->ctx_size, metadata->type)<  0) {
> 
> This mechanism makes it easier to add new functions.
> Several metadata use 0x53 and it is needed later.

Could I get that in understandable?
klv.key[5] == 0x53 is true for all except one, and even if it was a
second table would not be that much additional code, without having
to rely on key[5] corresponding to what the code needs to do.
The alternative is to change the signature of mxf_read_primer_pack to
match mxf_read_local_tags.