[MPlayer-dev-eng] [PATCH] tv:// for win32

[Reimar Döffinger]

Hello,
On Tue, Oct 09, 2007 at 04:33:49PM +0700, Vladimir Voroshilov wrote:
> 2007/9/8, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> > [...]
> > A proper review will hopefully follow some other day soon... ;-)
> 
> I'm still waiting ;)

I think I lack the time for that, but I do have a few comments.
> +#define OLE_CALL(p,method)               (p)->lpVtbl->method(p)
> +#define OLE_CALL1(p,method,a1)           (p)->lpVtbl->method(p,a1)
> +#define OLE_CALL2(p,method,a1,a2)        (p)->lpVtbl->method(p,a1,a2)

etc.

I think you could and should do that like e.g. codec_cfg.c:
> #ifdef __GNUC__
> #define mp_msg(t, l, m, args...) fprintf(stderr, m, ##args)
> #else
> #define mp_msg(t, l, ...) fprintf(stderr, __VA_ARGS__)
> #endif

I.e.
> #ifdef __GNUC__
> #define OLE_CALL_ARGS(p, method, a1, args...) (p)->lpVtbl->method(p, a1, ##args)
> #else
> #define OLE_CALL_ARGS(p, method, ...) (p)->lpVtbl->method(p, __VA_ARGS__)
> #endif

You will still need the OLE_CALL variant since the non-gnu version does
not allow empty __VA_ARGS__.
You could avoid this by explicitly adding the p argument instead of the
dummy a1 above, not sure which is better though.

> grabber_ringbuffer_s

should be grabber_ringbuffer_t, at least in MPlayer the convention is
"typedef struct something_s something_t".

The priv_t comments are also very messy for me, did you use tabs to
align them? I'd prefer if they were aligned with spaces, tabwidth simply
is different everywhere for me, so it never looks the same...

The physcon2str and chanlist2country functions should use lookup tables
instead of huge switch-cases.

And lastly, are you sure "#pragma pack" works with gcc? Even if it does
I think it would be preferable if you could find the corresponding
attributes and use them, but I'll leave that up to you to decide in the
end (as long as they work with gcc and don't give ugly warnings at
least).

Greetings,
Reimar Döffinger