From: Zebediah Figura <zfigura@codeweavers.com>
Subject: Re: [PATCH] include : Add dvdif.idl
Message-Id: <339e0e02-f5a4-bb8a-6089-aecaa13b377e@codeweavers.com>
Date: Thu, 9 Apr 2020 11:13:06 -0500
In-Reply-To: <CACfa+KL_Ptt+3HVbWUezbM7P=1XQaGARW8ai2YCDC5BWKcL=Lg@mail.gmail.com>
References: <CACfa+KL_Ptt+3HVbWUezbM7P=1XQaGARW8ai2YCDC5BWKcL=Lg@mail.gmail.com>

On 4/9/20 10:23 AM, Vijay Kiran Kamuju wrote:
> +typedef enum
> +{
> +    DVD_ResetOnStop                     = 1,
> +    DVD_NotifyParentalLevelChange       = 2,
> +    DVD_HMSF_TimeCodeEvents             = 3,
> +    DVD_AudioDuringFFwdRew              = 4,
> +    DVD_EnableNonblockingAPIs           = 5,
> +    DVD_CacheSizeInMB                   = 6,
> +    DVD_EnablePortableBookmarks         = 7,
> +    DVD_EnableExtendedCopyProtectErrors = 8,
> +    DVD_NotifyPositionChange            = 9,
> +    DVD_IncreaseOutputControl           = 10,
> +    DVD_EnableStreaming                 = 11,
> +    DVD_EnableESOutput 	                = 12,

Stray spaces here.

> +    DVD_EnableTitleLength               = 13,
> +    DVD_DisableStillThrottle            = 14,
> +    DVD_EnableLoggingEvents             = 15,
> +    DVD_MaxReadBurstInKB                = 16,
> +    DVD_ReadBurstPeriodInMS             = 17,
> +    DVD_RestartDisc                     = 18,
> +    DVD_EnableCC                        = 19
> +} DVD_OPTION_FLAG;

...

> +cpp_quote("#include <pshpack1.h>")
> +typedef struct tagDVD_AudioAttributes
> +{
> +    DVD_AUDIO_APPMODE AppMode;
> +    BYTE  AppModeData;
> +    DVD_AUDIO_FORMAT AudioFormat;
> +    LCID  Language;
> +    DVD_AUDIO_LANG_EXT LanguageExtension;
> +    BOOL  fHasMultichannelInfo;
> +    DWORD dwFrequency;
> +    BYTE  bQuantization;
> +    BYTE  bNumberOfChannels;
> +    DWORD dwReserved[2];
> +} DVD_AudioAttributes;
> +
> +typedef struct tagDVD_KaraokeAttributes
> +{
> +    BYTE bVersion;
> +    BOOL fMasterOfCeremoniesInGuideVocal1;
> +    BOOL fDuet;
> +    DVD_KARAOKE_ASSIGNMENT ChannelAssignment;
> +    WORD wChannelContents[8];
> +} DVD_KaraokeAttributes;
> +
> +typedef struct tagDVD_TitleMainAttributes
> +{
> +    union
> +    {
> +        DVD_TITLE_APPMODE AppMode;
> +        DVD_HMSF_TIMECODE TitleLength;
> +    } DUMMYUNIONNAME;
> +
> +    DVD_VideoAttributes VideoAttributes;
> +    ULONG ulNumberOfAudioStreams;
> +    DVD_AudioAttributes  AudioAttributes[8];
> +    DVD_MultichannelAudioAttributes MultichannelAudioAttributes[8];
> +    ULONG ulNumberOfSubpictureStreams;
> +    DVD_SubpictureAttributes SubpictureAttributes[32];
> +} DVD_TitleAttributes;
> +cpp_quote("#include <poppack.h>")

I don't see any packing directives in Microsoft's headers for these
structures. In fact, the Windows 10 SDK includes directives to ignore
padding warnings.

...

> +typedef struct tagDVD_DECODER_CAPS
> +{
> +    DWORD  dwSize;
> +    DWORD  dwAudioCaps;
> +    double dFwdMaxRateVideo;
> +    double dFwdMaxRateAudio;
> +    double dFwdMaxRateSP;
> +    double dBwdMaxRateVideo;
> +    double dBwdMaxRateAudio;
> +    double dBwdMaxRateSP;
> +    DWORD  dwRes1 ;
> +    DWORD  dwRes2 ;

Stray spaces.

> +    DWORD  dwRes3;
> +    DWORD  dwRes4;
> +} DVD_DECODER_CAPS;

...

> +[
> +  local,
> +  object,
> +  uuid(a70efe61-e2a3-11d0-a9be-00aa0061be93),
> +  pointer_default(unique)

Can you please keep indentation at a consistent 4 spaces?

> +]
> +interface IDvdControl : IUnknown
> +{
> +    HRESULT TitlePlay(
> +      [in] ULONG ulTitle);

Please avoid Hungarian notation for parameter names outside of library
blocks. There's also no need to put each parameter on a separate line,
or to put spaces between each function.

> +
> +    HRESULT ChapterPlay(
> +      [in] ULONG ulTitle,
> +      [in] ULONG ulChapter);

...

> +    HRESULT GetRoot(
> +      [out, size_is(ulBufSize)] LPSTR *pRoot,

This isn't a (double) pointer in my copy of the PSDK.

> +      [in] ULONG ulBufSize,
> +      [out] ULONG *pulActualSize);
> +}

...

> +    HRESULT GetCmdFromEvent(
> +      [in] ULONG_PTR lParam1,

This is LONG_PTR in my copy of the PSDK.

> +      [out] IDvdCmd **pCmdObj);
> +
> +    HRESULT GetDefaultMenuLanguage(
> +	  [out] LCID *pLanguage);
> +
> +    HRESULT GetDefaultAudioLanguage(
> +      [out] LCID *pLanguage,
> +      [out] DVD_AUDIO_LANG_EXT *pAudioExtension);
> +
> +    HRESULT SelectDefaultSubpictureLanguage(
> +      [out] LCID *pLanguage,
> +      [out] DVD_SUBPICTURE_LANG_EXT *pSubpictureExtension);
> +
> +    HRESULT GetDecoderCaps(
> +      [out] DVD_DECODER_CAPS *pCaps);
> +
> +    HRESULT GetButtonRect(
> +      [in] ULONG ulButton,
> +      [out] RECT *pRect);
> +
> +    HRESULT IsSubpictureStreamEnabled(
> +      [in] ULONG ulStreamNum,
> +      [out] BOOL *pbEnabled);
> +}
> +
> +[
> +local,
> +object,
> +uuid(fcc152B6-f372-11d0-8e00-00c04fd7c08b),
> +pointer_default(unique)

Please indent these attributes, and avoid mixing case in GUID declarations.

> +]
> +interface IDvdGraphBuilder : IUnknown
> +{
> +    HRESULT GetFiltergraph(
> +      [out] IGraphBuilder **ppGB);
> +
> +    HRESULT GetDvdInterface(
> +      [in] REFIID riid,
> +      [out] void **ppIF);
> +
> +    HRESULT RenderDvdVideoVolume(
> +      [in] LPCWSTR lpcwszPathName,
> +      [in] DWORD dwFlags,
> +      [out] AM_DVD_RENDERSTATUS *pStatus);
> +}
> diff --git a/include/strmif.idl b/include/strmif.idl
> index a06f99b751..38af866f47 100644
> --- a/include/strmif.idl
> +++ b/include/strmif.idl
> @@ -27,5 +27,6 @@ import "objidl.idl";
>  
>  #include <dyngraph.idl>
>  
> -/* TODO: #include <dvdif.h> */
>  #include <vmrender.idl>
> +
> +#include <dvdif.idl>
> 
> -- 
> 2.26.0
>