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

[Reimar Döffinger]

On Tue, Jun 29, 2010 at 04:47:32PM -0700, Baptiste Coudurier wrote:
> On 6/29/10 2:35 PM, Reimar D?ffinger wrote:
> >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.
> 
> And when you add a new function, you have to look up for the define
> to know which parameters are available. This is even more painful.

Have you actually _tried_ changing the parameters of all those functions?
I had to pick each single one from the table, search for it's declaration
and then change it.
Searching for the define to get the argument names is quite simple in
comparison.
But you have to maintain it, I'll change it if that's the only objection.

> >>>@@ -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.
> 
> The contrary, next time we add a function parsing metadata not using
> 0x53, we will need an ugly if (key), and index tables do not use
> 0x53 for example.

As said, a second table is possible, or it is possible to stay
closer to the current code with attached patch.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mxfdec.diff
Type: text/x-diff
Size: 4934 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100630/536d293b/attachment.diff>