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

[Vladimir Voroshilov]

Hi, Reimar.

Thank you for your review.

2007/10/9, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> 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.

Should i assume that remaining code is acceptable for commit (IOW:
other issues, if any,
can be fixed after commit)?

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

Fixed with OLE_CALL_ARGS and OLE_CALL.

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

Fixed.

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

Yes. i did. All tabs are replaced with spaces.

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

Done.

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

Yes it works,
But consistant style is better, so i've changed structure definition
as done in loader/dshow/mediatype.h

-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dshow_core_hdr_v101_2.diff.gz
Type: application/x-gzip
Size: 5492 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071009/a55468ab/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dshow_core_v101_2.diff.gz
Type: application/x-gzip
Size: 23925 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071009/a55468ab/attachment-0001.bin>