[MPlayer-dev-eng] [PATCH] VCD support for OS/2

[KO Myung-Hun]

Hi/2.

Reimar Döffinger wrote:
> On Wed, Jan 14, 2009 at 01:10:47AM +0900, KO Myung-Hun wrote:
>   
>> On OS/2, DosDevIOCtl() expect a packed struct or a byte buffer. So I 
>> decide to use '#pragma pack' globally because all structs are for 
>> DosDevIOCtl() except 'mp_vcd_priv_t'. And the system headers for them do 
>> not exist.
>>     
>
> So with pragma you now have:
> 1) specified packed in a way that a (hypothetical) compiler which does
> not support it will most likely ignore, thus failing at runtime instead
> of compiletime
>   

Why do you always assume in that way ? I know, all the compiler for OS/2 
supports '#pragma pack' directives, and so do they in the future.

> 2) made all structs declared in the file packed, which is currently
> not really good in case of mp_vcd_priv_t and can cause hard to debug issues if
> someone is ever so careless to include a .h file after the pragma,
> while at the same time only a single struct actually _needs_ to be
> packed explicitly.
>   

Why do 'packed structs' make debug to hard ? And why does a single 
struct need to be packed explicitly ? I think you missed '#pragma 
pack()' in the end of file.

> in the case of sParamDisk, the struct is completely pointless and makes
> the initialization needlessly hard to read.
>   

In that case, we can use a byte buffer. But using struct is not a bad 
choice. It's just a problem of favor. And I wanted to maintain 
consistency if possible.

Why hard to read the initialization ? I don't understand.

> In the case of sParam and sParamTrack the struct type declaration code
> is duplicated, and I don't agree that
>   

Hmmm... Why should not the declaration code be duplicated ? That cause 
any problem ? Or just do you dislike it ?

>>    struct {
>>       UCHAR   auchSign[4];
>>       BYTE    bTrack;
>>    } sParamTrack = {{'C', 'D', '0', '1'},};
>>     
> [...]
>   
>> sParamTrack.bTrack = i;
>>     
>
> is better for readability than one
>
>   
>> uint8_t track[5] = {'C', 'D', '0', '1', i};
>>     
>
>   

What do you mean by 'readability' ?

> Which also avoids the assumption that DosDevIOCtl will not modify this
> parameter, it is not marked as "const" in the function prototype after
> all.
>
>   

You're right. But it depends on ioctl category. At least, in case of CD 
IO, 'param' arg is used as input and 'data' args is used as input/output.

>> Of course, I absolutely agree with you if these codes are for general 
>> platforms and OSes. But these are only for OS/2. And those structs are 
>> defined in according to OS/2 API DOCs. Why should I consider what you said ?
>>     
>
> Are those OS/2 API docs publicly available somewhere?
>   

You can use 'http://www.warpspeed.com.au/book/books.htm' as well as what 
Dave said.

Refer to CP1.INF for DosDevIOCtl() and to 'Category 80h' and 'Category 
81h' of 'Generic IOCtl Commands' CP2.INF for CD IOCTL.

>   
>> And I don't understand a byte buffer has a better readability than a 
>> structure. If not using a structure, how can I know the meaning of each 
>> index ?
>>     
>
> How can you know the meaning with names like auchSign? I consider the
> non-struct version as in the example I gave above more readable.
>
>   

If so, which is more meaningful and readable between sParamTrack.bTrack 
and track[4] ?

>> And just a question. Is there a compiler not supporting 'packed structure' ?
>>     
>
> Probably not, but as I remember it early gcc versions did not support
> #pragma pack, in general each compiler tends to have their own way of
> specifying it - per-struct declarations usually can be fixed by using a
> macro instead, whereas #pragmas generally either work or they don't and
> usually there is no way to even find out.
>
>   
Are you sure that all the compiler support per-struct declaration ? 
Actually, 'packed' attribute is ignored on GCC port for OS/2, but all 
the compilers for OS/2 accept '#pragma pack' directive.

>> Yes, many compiler set the default stack size for OS/2 to 32K or 64K. 
>> But I think that size is enough to use 2K stack variable. In addition, 
>> GCC 4.3.2 are being ported to OS/2. It set the default stack size to 1M. 
>> Currently I'm using that GCC 4.3.2. Nevertheless I'm setting the stack 
>> size to 16M for MPlayer. So I think your apprehension is unnecessary.
>>     
>
> And why not just put it in the mp_vcd_priv_t that you already have
> anyway, thus making sure it will _certainly_ work for "any" stack size?
>
>   

Ok. Good idea.

Fixed.

-- 
KO Myung-Hun

Using Mozilla SeaMonkey 1.1.14
Under OS/2 Warp 4 for Korean with FixPak #15
On AMD ThunderBird 1 GHz with 512 MB RAM

Korean OS/2 User Community : http://www.ecomstation.co.kr


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: vcd.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20090114/e89eac72/attachment.txt>