Fwd: Re: kernel32/tests: Add tests for job objects (try 7)
(no idea why my client sent this to wine-patches) On 10/10/13 15:23, Andrew Cook wrote: > --- > dlls/kernel32/tests/process.c | 159 > +- > include/winbase.h | 1 + > include/winnt.h | 90 > 3 files changed, 249 insertions(+), 1 deletion(-) > > there appears to be a window between the all processes leaving a job and the job actually being considered empty, and no apparent way to wait for this. I can't reproduce this locally either. Is there an accepted way to wait for some inaccessible kernel-side event? reordering some calls would likely hide the issue but i'd rather avoid that. https://newtestbot.winehq.org/JobDetails.pl?Key=2687 failure on w8 https://newtestbot.winehq.org/JobDetails.pl?Key=2709 failure on w864 https://newtestbot.winehq.org/JobDetails.pl?Key=2685 same patch, success on w864
Re: rpcss: Convert to a service
Ignore this, for some reason this randomly forks a huge number of rpcss instances, and decided to not do so while i was testing it On 03/10/13 14:00, Andrew Cook wrote: > --- > dlls/rpcrt4/rpc_epmap.c | 40 --- > programs/rpcss/Makefile.in | 2 +- > programs/rpcss/rpcss_main.c | 97 > +++-- > tools/wine.inf.in | 11 + > 4 files changed, 104 insertions(+), 46 deletions(-) > >
Re: [PATCH] dsound: fixup IDirectSoundCaptureBuffer_QueryInterface
Seems reasonable, but could you put together some tests to show this?
Andrew
On Sat, Sep 28, 2013 at 10:41:15AM +0200, Maarten Lankhorst wrote:
> diff --git a/dlls/dsound/capture.c b/dlls/dsound/capture.c
> index 40f1702..0fe300c 100644
> --- a/dlls/dsound/capture.c
> +++ b/dlls/dsound/capture.c
> @@ -51,7 +51,7 @@ typedef struct IDirectSoundCaptureBufferImpl
> IDirectSoundCaptureBuffer8 IDirectSoundCaptureBuffer8_iface;
> IDirectSoundNotify IDirectSoundNotify_iface;
> LONGnumIfaces; /* "in use interfaces"
> refcount */
> -LONGref, refn;
> +LONGref, refn, has_dsc8;
> /* IDirectSoundCaptureBuffer fields */
> DirectSoundCaptureDevice*device;
> DSCBUFFERDESC *pdscbd;
> @@ -241,8 +241,9 @@ static HRESULT WINAPI
> IDirectSoundCaptureBufferImpl_QueryInterface(IDirectSoundC
>
> *ppobj = NULL;
>
> -if ( IsEqualGUID( &IID_IDirectSoundCaptureBuffer, riid ) ||
> - IsEqualGUID( &IID_IDirectSoundCaptureBuffer8, riid ) ) {
> +if ( IsEqualIID( &IID_IUnknown, riid ) ||
> + IsEqualIID( &IID_IDirectSoundCaptureBuffer, riid ) ||
> + (This->has_dsc8 && IsEqualIID( &IID_IDirectSoundCaptureBuffer8,
> riid )) ) {
> IDirectSoundCaptureBuffer8_AddRef(iface);
> *ppobj = iface;
> return S_OK;
> @@ -1239,6 +1240,8 @@ static HRESULT WINAPI
> IDirectSoundCaptureImpl_CreateCaptureBuffer(IDirectSoundCa
>
> if (hr != DS_OK)
> WARN("IDirectSoundCaptureBufferImpl_Create failed\n");
> +else
> +This->device->capture_buffer->has_dsc8 = This->has_dsc8;
>
> return hr;
> }
>
>
>
Re: ntdll/tests: Add tests for job objects. (try 2)
On 06/09/13 17:12, Frédéric Delanoy wrote: > On Fri, Sep 6, 2013 at 5:41 AM, Andrew Cook wrote: >> --- >> dlls/ntdll/tests/Makefile.in | 1 + >> dlls/ntdll/tests/job.c | 151 >> +++ >> include/winnt.h | 5 ++ >> 3 files changed, 157 insertions(+) >> create mode 100644 dlls/ntdll/tests/job.c > > +if(pNtIsProcessInJob) > +todo_wine todo_wine ok(pNtIsProcessInJob(pi[0].hProcess, > JobObject) == STATUS_PROCESS_NOT_IN_JOB, > +"NtIsProcessInJob: expected STATUS_PROCESS_NOT_IN_JOB, > got %x\n", ret); > > Is that double todo_wine intended? It seems weird... > I have no idea how i missed that. probably better this patch is disregarded, i've sent an equivalent set of tests for kernel32.
Re: ntdll: Implement job objects
On 27/08/13 22:02, Dmitry Timoshkov wrote: > Andrew Cook wrote: >> --- a/include/wine/server_protocol.h >> +++ b/include/wine/server_protocol.h > > And don't include autogenerated stuff in the patch. > Is there anything about this on the wiki? i wasn't sure how autogenerated files are supposed to be handled.
Re: RFC: winedev guide multimedia chapter patch
Looks good for the most part. Some thoughts below... On Sat, Aug 17, 2013 at 01:25:00PM +0200, Frédéric Delanoy wrote: > + The implementation can be found in a number of directories: > + > + > + > +dlls/winmm/, +class="directory">dlls/mmsystem.dll16/ and +class="directory">dlls/msacm32.drv/ > + > + > + > + > +dlls/msacm32/ and > +class="directory">dlls/msacm.dll16/ (for the audio > compression/decompression > +manager) > + > + > + > + > +dlls/msvfw32/ and > +class="directory">dlls/msvideo.dll16/ (for the video > +compression/decompression manager). > + > + > + Most of these paths are not very relevant. The 16-bit DLLs are really just wrappers around the full DLLs to convert into 16-bit types. I would list, in order of importance, mmdevapi, dsound, winmm, msacm32, and msvfw32. The first three are the main audio APIs. dsound and winmm are implemented on top of mmdevapi, and that is implemented in the three driver DLLs, winealsa.drv, winecoreaudio.drv, and wineoss.drv msacm32 deals with converting between audio formats, and msvfw32 deals with displaying videos. Both of those use external DLLs like mciavi32, or even DLLs supplied by applications, to do conversions. > +The low level layer has also some helper DLLs (like the > msacm32/msacm.dll16 > +and msvfw32/msvideo.dll16 pairs). Again, I wouldn't bother listing the 16-bit DLLs at all, through the whole patch. They aren't interesting. > @@ -486,7 +502,7 @@ Kernel space |Client applications > Note that a Wine specific flag has been added to the > wodOpen function, so that the DSound > DLL can get a reference to a COM sound object from a given > - WINMM wave output device. This should be changed in the > + winmm wave output device. This should be changed in the > future. > This whole paragraph can actually be removed. The dsound object it refers to no longer exists. Thanks for updating this, Andrew
Re: winmm: fix WOD_Open() WAVE_MAPPER no-device case.
On Tue, Aug 20, 2013 at 07:06:13PM +0200, Fabrice Bauzac wrote:
> +if (g_inmmdevices_count == 0) {
> +return MMSYSERR_BADDEVICEID;
> +}
Looks like the indentation is bad here.
Also please submit the patch to wine-patches. Patches aren't picked up
from wine-devel.
Thanks for the fix!
Andrew
Re: winmm: fix WOD_Open() WAVE_MAPPER no-device case.
On Tue, Aug 20, 2013 at 05:57:34AM +0200, Fabrice Bauzac wrote: > Hello, > > This patch intends to fix the bug: > http://bugs.winehq.org/show_bug.cgi?id=34305 > > The bug: basically, when there is no sound device and the user tries > to WOD_Open() the WAVE_MAPPER pseudodevice, the code calls read_map() > with device index 0 without first checking whether the number of > devices is greater than 0. > Looks good to me, thanks. I tested on Windows 7, and the error code is correct. By the way, the waveIn path (WID_Open) has the same bug. Feel free to fix that, too, if you want. Or I'll submit a patch myself later. Andrew
Re: Process for reporting security bugs?
Hi Marcus, >If it is not a high severe issue you can also just mail this mailinglist >here (wine-devel). Thanks for the info. As it turns out, it's an already-known issue (unixfs allows full host filesystem access through Windows APIs even if there's no equivalent dosdevices link -- reported as http://bugs.winehq.org/show_bug.cgi?id=22450) so I just added a comment onto the bug. Thanks, --Andrew Church achu...@achurch.org http://achurch.org/
Re: Process for reporting security bugs?
>Depending on what attack scenario you envision, disabling unixfs is not enough. > >If you want to avoid actually executed malware from accessing the UNIX fs >directly, >you are out of luck as the malware could just do systemcalls itself (int 0x80 >on x86 >for instance). Yup, I'm aware of that -- if I'm going to run potentially malicious code I'll run it in a VM. (: My concern is more for cases like otherwise well-behaved programs that passively report to some remote server what they see, such as when listing folders via the shell interface -- ideally I'd like to limit what they see without the overhead of setting up a full VM. Might it be worth adding a note about both unixfs and the syscall issue to the documentation? The only reference I found to host filesystem access is under section 4.1.4 "Drive Settings". Maybe add some text like: """ Wine also provides a Windows share which allows modern applications to access native Unix paths even without a drive mapping. This will show up as "/" in file dialogs. Note that removing the default "z:" drive mapping will NOT prevent Windows applications from reading your entire filesystem! In addition to the Windows share, malicious programs could detect that they are running under Wine and execute native Linux system calls to get around any restrictions imposed by Wine. Consider running programs you don't trust in a virtual machine instead. """ Just a thought, --Andrew Church achu...@achurch.org http://achurch.org/
Process for reporting security bugs?
[Please cc: me on any replies since I'm not subscribed to the list.] Hi, Is there a specific process for reporting security-related bugs in Wine? I've looked through winehq.org but haven't found any mention of such; I just wanted to make sure I haven't overlooked anything before posting the bug in a publicly visible manner. Thanks, --Andrew Church achurch+wine-de...@achurch.org http://achurch.org/
Re: [2/2] wineserver: perform access check when creating event objects (try 2)
Actually ignore this patch, the problem was introduced by 92db6d2c back in 2007, server: Don't do access checks on the security descriptors of newly created objects which fixes sync.c:363 "CreateEventW with blank sd failed" i'll need to take another look at this, calling CreateEventEx with generic flags fails to correctly set permissions, the patch as it stands though will likely cause regressions. On 02/08/13 21:58, Andrew Cook wrote: > --- > dlls/ntdll/tests/om.c | 4 ++-- > server/event.c| 5 + > 2 files changed, 3 insertions(+), 6 deletions(-) > >
Re: winmm: Standardize and simplify two error messages.
On Wed, Jul 10, 2013 at 10:42:05AM +0200, Francois Gouget wrote: > We don't have a MIDI Mapper configuration applet so don't tell the user to > use it. > --- > > Or if we do, how does one get to it? I did not find anything in > winecfg.exe, nor in control.exe (the Control Panel). > > -MIDIERR_NODEVICE,"The current MIDI Mapper setup refers to a MIDI > device that is not installed on the system. Use MIDI Mapper to edit the > setup." I think it's meant to refer to the "MIDI Mapper" device, which is configurable through the registry: http://wiki.winehq.org/MIDI I think your change is clearly an improvement. Andrew
Re: po: Update Simplified Chinese translation. (newer)
It comes basically from the ERole enumeration in MMDevAPI: http://msdn.microsoft.com/en-us/library/windows/desktop/dd370842%28v=vs.85%29.aspx The "Input device" maps to eConsole (and eMultimedia, which is identical to eConsole in modern Windows). The "Voice input device" maps to eCommunications. So, "human voice" is the intended meaning. Hope this clears it up! Andrew On Fri, Jul 05, 2013 at 11:43:23PM +0800, Qian Hong wrote: > Hi Andrew, > > While discussing the translation of 'Voice output device' and 'Voice > input device' in winecfg, Jactry and I have no idea what does 'voice' > mean here. > Voice could be translated to both '声音‘ and '语音' in Chinese, the former > means all kinds of sound, the latter means human voice only, which > one is more accurate? > > I know you've subscribed wine-devel, I CC'ed you because you are the > original contributor to the 'voice input device' related code. Thanks > a lot! > > On Fri, Jul 5, 2013 at 9:53 PM, Jactry Zeng wrote: > > Please ignore the first patch, I modify some in this one. > > Thanks advice from Qian Hong. > > > > > > > > > > -- > Regards, > Qian Hong > > - > http://www.winehq.org
Re: Add tests for ntdll event functions
While implemeting NtQueryEvent i stumbled across an odd permissions
issue, which is really what this patch tests
if ((event = create_event( root, &name, req->attributes,
req->manual_reset, req->initial_state, sd )))
{
if (get_error() == STATUS_OBJECT_NAME_EXISTS)
reply->handle = alloc_handle( current->process, event,
req->access, req->attributes );
else
reply->handle = alloc_handle_no_access_check(
current->process, event, req->access, req->attributes );
release_object( event );
}
when creating an event, the handle is allocated with
alloc_handle_no_access_check, which does not call event_map_access
this causes event_op and event_query to fail, as the handle only has
GENERIC_ALL, not the actual event permissions,
which does not match windows behaviour (or at least windows 7 anyway),
On 04/06/13 11:13, Andrew Cook wrote:
> ---
> dlls/ntdll/tests/Makefile.in | 1 +
> dlls/ntdll/tests/event.c | 69
>
> 2 files changed, 70 insertions(+)
> create mode 100644 dlls/ntdll/tests/event.c
>
> diff --git a/dlls/ntdll/tests/Makefile.in b/dlls/ntdll/tests/Makefile.in
> index 10d6674..a44b880 100644
> --- a/dlls/ntdll/tests/Makefile.in
> +++ b/dlls/ntdll/tests/Makefile.in
> @@ -7,6 +7,7 @@ C_SRCS = \
> directory.c \
> env.c \
> error.c \
> + event.c \
> exception.c \
> file.c \
> generated.c \
> diff --git a/dlls/ntdll/tests/event.c b/dlls/ntdll/tests/event.c
> new file mode 100644
> index 000..f29abc3
> --- /dev/null
> +++ b/dlls/ntdll/tests/event.c
> @@ -0,0 +1,69 @@
> +/*
> + * Unit test suite for event functions
> + *
> + * Copyright 2005 Robert Shearman
> + * Copyright 2005 Vitaliy Margolen
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> 02110-1301, USA
> + */
> +
> +#include "ntdll_test.h"
> +#include "winternl.h"
> +#include "winnt.h"
> +
> +static NTSTATUS (WINAPI *pNtCreateEvent) ( PHANDLE, ACCESS_MASK, const
> POBJECT_ATTRIBUTES, BOOLEAN, BOOLEAN);
> +static NTSTATUS (WINAPI *pNtPulseEvent) ( HANDLE, PULONG );
> +static NTSTATUS (WINAPI *pNtQueryEvent) ( HANDLE,
> EVENT_INFORMATION_CLASS, PVOID, ULONG, PULONG );
> +static NTSTATUS (WINAPI *pNtClose) ( HANDLE );
> +static VOID (WINAPI *pRtlInitUnicodeString)( PUNICODE_STRING,
> LPCWSTR );
> +
> +START_TEST(event)
> +{
> +HANDLE Event;
> +NTSTATUS status;
> +UNICODE_STRING str;
> +OBJECT_ATTRIBUTES attr;
> +EVENT_BASIC_INFORMATION info;
> +HMODULE hntdll = GetModuleHandleA("ntdll.dll");
> +HMODULE hkernel32 = GetModuleHandleA("kernel32.dll");
> +static const WCHAR eventName[] =
> {'\\','B','a','s','e','N','a','m','e','d','O','b','j','e','c','t','s','\\','t','e','s','t','E','v','e','n','t',0};
> +
> +if (!hntdll)
> +{
> +skip("not running on NT, skipping test\n");
> +return;
> +}
> +
> +pNtCreateEvent = (void *)GetProcAddress(hntdll, "NtCreateEvent");
> +pNtQueryEvent = (void *)GetProcAddress(hntdll, "NtQueryEvent");
> +pNtPulseEvent = (void *)GetProcAddress(hntdll, "NtPulseEvent");
> +pNtClose = (void *)GetProcAddress(hntdll, "NtClose");
> +pRtlInitUnicodeString = (void *)GetProcAddress(hntdll,
> "RtlInitUnicodeString");
> +
> +pRtlInitUnicodeString(&str, eventName);
> +InitializeObjectAttributes(&attr, &str, 0, 0, NULL);
> +
> +status = pNtCreateEvent(&Event, GENERIC_ALL, &attr, 0, 0);
> +ok( status == STATUS_SUCCESS, "NtCreateEvent failed %08x\n", status );
> +
> +status = pNtPulseEvent(Event, 0);
> +todo_wine ok( status == STATUS_SUCCESS, "NtPulseEvent failed
> %08x\n", status );
> +
> +status = pNtQueryEvent(Event, EventBasicInformation, &info,
> sizeof(info), NULL);
> +todo_wine ok( status == STATUS_SUCCESS, "NtQueryEvent failed
> %08x\n", status );
> +
> +status = pNtClose(Event);
> +ok( status == STATUS_SUCCESS, "NtClose failed %08x\n", status );
> +}
Re: [PATCH (try 2)] dsound: Copy SetFormat format more directly if we're not in PRIMARYWRITE mode
On Wed, May 29, 2013 at 01:10:56PM +0200, Maarten Lankhorst wrote: > NAK. > > + HeapFree(GetProcessHeap(), 0, device->primary_pwfx); > + device->primary_pwfx = DSOUND_CopyFormat(passed_fmt); > Copy to a tmp buffer first and return E_OUTOFMEMORY instead of reassigning > primary_pwfx to NULL? > CopyFormat will only return NULL if HeapAlloc fails (actually, CopyFormat will probably crash). I've stopped caring about out of memory conditions since they add way too much error handling code and we can't reasonably recover from them anyway. Andrew
Re: [PATCH 2/5] dinput: Support SendForceFeedbackCommand for OSX joysticks
On Tue, May 21, 2013 at 02:52:55PM -0600, Charles Davis wrote: > You can't return the straight HRESULT from ForceFeedback here. That's because > the codes in the FACILITY_NULL range on Mac get their values from the 16-bit > COM runtime (cf. ). You must turn them into > their corresponding Win32 codes. > Interesting, thanks. I'll go back and fix this. Andrew
Re: [PATCH 2/3] oleaut32: COM cleanup for ITypeInfo object
Looks like I missed replacing the ITypeInfo casts with impl_from
calls, e.g.:
static HRESULT WINAPI ITypeInfo_fnGetTypeComp( ITypeInfo2 *iface,
ITypeComp * *ppTComp)
{
ITypeInfoImpl *This = (ITypeInfoImpl *)iface;
You can apply these, or wait until I amend them to fix that, too.
Whichever.
Andrew
On Wed, May 08, 2013 at 03:12:45PM -0500, Andrew Eikum wrote:
> ---
> dlls/oleaut32/typelib.c | 30 +++---
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/dlls/oleaut32/typelib.c b/dlls/oleaut32/typelib.c
> index e62fa84..c6d2189 100644
> --- a/dlls/oleaut32/typelib.c
> +++ b/dlls/oleaut32/typelib.c
> @@ -1094,8 +1094,8 @@ typedef struct tagTLBImplType
> /* internal TypeInfo data */
> typedef struct tagITypeInfoImpl
> {
> -const ITypeInfo2Vtbl *lpVtbl;
> -const ITypeCompVtbl *lpVtblTypeComp;
> +ITypeInfo2 ITypeInfo2_iface;
> +ITypeComp ITypeComp_iface;
> LONG ref;
> BOOL not_attached_to_typelib;
> TYPEATTR TypeAttr ; /* _lots_ of type information. */
> @@ -1125,7 +1125,7 @@ typedef struct tagITypeInfoImpl
>
> static inline ITypeInfoImpl *info_impl_from_ITypeComp( ITypeComp *iface )
> {
> -return (ITypeInfoImpl *)((char*)iface - FIELD_OFFSET(ITypeInfoImpl,
> lpVtblTypeComp));
> +return CONTAINING_RECORD(iface, ITypeInfoImpl, ITypeComp_iface);
> }
>
> static const ITypeInfo2Vtbl tinfvt;
> @@ -4884,7 +4884,7 @@ static HRESULT WINAPI ITypeLibComp_fnBind(
> if (pTypeInfo->Name && !strcmpW(pTypeInfo->Name, szName))
> {
> *pDescKind = DESCKIND_TYPECOMP;
> -pBindPtr->lptcomp = (ITypeComp *)&pTypeInfo->lpVtblTypeComp;
> +pBindPtr->lptcomp = &pTypeInfo->ITypeComp_iface;
> ITypeComp_AddRef(pBindPtr->lptcomp);
> TRACE("module or enum: %s\n", debugstr_w(szName));
> return S_OK;
> @@ -4894,7 +4894,7 @@ static HRESULT WINAPI ITypeLibComp_fnBind(
> if ((pTypeInfo->TypeAttr.typekind == TKIND_MODULE) ||
> (pTypeInfo->TypeAttr.typekind == TKIND_ENUM))
> {
> -ITypeComp *pSubTypeComp = (ITypeComp
> *)&pTypeInfo->lpVtblTypeComp;
> +ITypeComp *pSubTypeComp = &pTypeInfo->ITypeComp_iface;
> HRESULT hr;
>
> hr = ITypeComp_Bind(pSubTypeComp, szName, lHash, wFlags,
> ppTInfo, pDescKind, pBindPtr);
> @@ -4910,7 +4910,7 @@ static HRESULT WINAPI ITypeLibComp_fnBind(
> if ((pTypeInfo->TypeAttr.typekind == TKIND_COCLASS) &&
> (pTypeInfo->TypeAttr.wTypeFlags & TYPEFLAG_FAPPOBJECT))
> {
> -ITypeComp *pSubTypeComp = (ITypeComp
> *)&pTypeInfo->lpVtblTypeComp;
> +ITypeComp *pSubTypeComp = &pTypeInfo->ITypeComp_iface;
> HRESULT hr;
> ITypeInfo *subtypeinfo;
> BINDPTR subbindptr;
> @@ -5013,9 +5013,9 @@ static HRESULT WINAPI ITypeLibComp_fnBindType(
> if (pTypeInfo->Name && !strcmpiW(pTypeInfo->Name, szName))
> {
> TRACE("returning %p\n", pTypeInfo);
> -*ppTInfo = (ITypeInfo *)&pTypeInfo->lpVtbl;
> +*ppTInfo = (ITypeInfo *)&pTypeInfo->ITypeInfo2_iface;
> ITypeInfo_AddRef(*ppTInfo);
> -*ppTComp = (ITypeComp *)&pTypeInfo->lpVtblTypeComp;
> +*ppTComp = &pTypeInfo->ITypeComp_iface;
> ITypeComp_AddRef(*ppTComp);
> return S_OK;
> }
> @@ -5046,8 +5046,8 @@ static ITypeInfoImpl* ITypeInfoImpl_Constructor(void)
> pTypeInfoImpl = heap_alloc_zero(sizeof(ITypeInfoImpl));
> if (pTypeInfoImpl)
> {
> - pTypeInfoImpl->lpVtbl = &tinfvt;
> - pTypeInfoImpl->lpVtblTypeComp = &tcompvt;
> + pTypeInfoImpl->ITypeInfo2_iface.lpVtbl = &tinfvt;
> + pTypeInfoImpl->ITypeComp_iface.lpVtbl = &tcompvt;
>pTypeInfoImpl->ref = 0;
>pTypeInfoImpl->hreftype = -1;
>pTypeInfoImpl->TypeAttr.memidConstructor = MEMBERID_NIL;
> @@ -5238,7 +5238,7 @@ static HRESULT WINAPI ITypeInfo_fnGetTypeComp(
> ITypeInfo2 *iface,
>
> TRACE("(%p)->(%p)\n", This, ppTComp);
>
> -*ppTComp = (ITypeComp *)&This->lpVtblTypeComp;
> +*ppTComp = &This->ITypeComp_iface;
> ITypeComp_AddRef(*ppTComp);
> return S_OK;
> }
> @@ -6999,7 +6999,7 @@ static HRESULT WINAPI ITypeInfo_fnGetRefTypeInfo(
>
> if ((This->hreftype != -1) && (This->hreftype == hRefType))
>
Re: msvcp60: Avoid signed-unsigned integer comparisons
larmbr zhan wrote: > On Sun, Mar 10, 2013 at 5:48 AM, Andrew Talbot > wrote: >> msvcp60: Avoid signed-unsigned integer comparisons. > > > Hi, Andrew Talbot. > > I find that you are working on these "Avoid signed-unsigned integer > comparisons" things recently. > > I DO agree on that using _ unsigned int _ instead of _ int _(which > implies _ signed int _). > But I think using size_t maybe more standard-compliant, more > efficient, and less bug-prone, . > > [...] Hi, Zhan JIanyu, I would say that size_t (and ptrdiff_t) are most suitable for "memsizes" that are open-ended and could potentially have huge values. However, where it is clear that gigabytes will never be involved nor billions of items, there is nothing more efficient than an int (signed or unsigned), since it has the same width as the processor. For real-world quantities, such as "width" and "height", I would prefer to use signed intS where possible, even for non-negative commodities. I would prefer unsigned intS for bit arrays. Regard, -- Andy.
Re: [PATCH] dsound: copy program parameters in SetFormat
On Mon, Feb 11, 2013 at 02:06:42PM +0100, Maarten Lankhorst wrote: > Fixes bug #32312, apparently some programs depend on the format it specifies > being returned. > > Just set wBitsPerSample, nSamplesPerSec and nChannels to make those > applications happy. > The patch seems fine, but can you add some tests for this so we can nail down exactly what we should be taking from the SetFormat format? E.g. should we just do "device->primary_fmt = CopyFormat(passed_fmt)" in all cases, or can you find a case where the GetFormat() result is changed? FYI, your patch applies with an offset and the Wine patch tracker didn't like this: > --- a/dlls/dsound/primary.c > +++ a/dlls/dsound/primary.c but git-am imported the raw mail file just fine. Andrew
Re: winmm: More compatible waveIn/Out[Un]Prepare WHDR_* flag handling.
These two patches look fine to me. Thanks! On Mon, Feb 11, 2013 at 01:28:09PM +0100, joerg-cyril.hoe...@t-systems.com wrote: > Hi, > > This completes my winmm Prepare Header changes on the wave side. > > XyzPrepare: > +header->dwFlags &= ~(WHDR_DONE|WHDR_INQUEUE); /* flags cleared since w2k > */ > > I choose to implement the behavior since w2k. Hopefully this will not break > some w9x app. > > > WINMM_UnprepareHeader: > -header->dwFlags |= WHDR_DONE; > Never ever observed. Neither w95 nor xp or w7. > Perhaps MS would have been wise to do so, as simply looking at a header you > cannot > tell whether it's been processed or not (returned with error code). > I choose to implement compatible behaviour. > > > -if(!(lpWaveInHdr->dwFlags & WHDR_PREPARED)) > -return MMSYSERR_NOERROR; > if(lpWaveInHdr->dwFlags & WHDR_INQUEUE) > return WAVERR_STILLPLAYING; > +if(!(lpWaveInHdr->dwFlags & WHDR_PREPARED)) > +return MMSYSERR_NOERROR; > Note the subtle difference in behaviour between wave and midi headers... > > > >IMHO calling named winmm:xyz functions should be equivalent to dispatching > >MM* messages > > I forgot to add that I'm still surprised that Andrew Eikum could reimplement > waveIn/OutMessage *without* using MMDRV_Message by simply returning > MMSYSERR_NOTSUPPORTED. > > Either all apps use the named winmm:waveIn/OutXyz functions instead of > calling waveOutMessage() > or nobody bothered to complain that some apps (perhaps w3.1 ones?) lack sound. > > That area obviously needs some testing... > > Regards, > Jörg Höhle
Re: RFC: Remove auto-scan of ALSA devices from winealsa.drv (and get repeatable behaviour)
On Tue, Jan 22, 2013 at 11:15:59AM +0100, joerg-cyril.hoe...@t-systems.com wrote: > Andrew Eikum was in favour of this too and since implemented winmm > device notification upon change. Remember the December thread: > http://www.winehq.org/pipermail/wine-devel/2012-December/098114.html > Yeah, I still think it's a good idea. Assuming we remove enumeration and add a good UI to manually add devices, there were two unanswered objections raised in the last thread. 1) We still want to do this, but want a better way to do this (i.e. work with upstream to make a good enumeration API) This still requires us to gut our existing enumeration, so I guess you can think of it in two pieces: remove the existing enumeration, and add "good" enumeration should that ever exist. So let's do step one now and maybe step two if it becomes possible. 2) Each new prefix will require manual configuration This is true now, but it would be a larger pain if the user has to add a device in addition to selecting it. Is there some existing mechanism for users to customize prefixes when they're created? Should there be? Anyway, I'll put this on my TODO list and let you know when I've got something working. Andrew
Re: [PATCH 1/5] dsound: rework ugly mixer logic
On Tue, Jan 15, 2013 at 06:51:59PM +0100, Maarten Lankhorst wrote: > Op 07-01-13 16:58, Andrew Eikum schreef: > > This patch breaks the sound on Zaxxon[1] on both CoreAudio and ALSA > > without PulseAudio. I didn't test OSS. It skips around on the ship's > > shooting noises. I've attached good and bad logs on ALSA here. > > > > [1] > > http://www.classic-retro-games.com/Zaxxon_70.html > > > I don't see anything suspicious in the log. Did it work after the complete > patch series was applied or did you not test? Yeah, I was testing with the whole series when I found the problem, and rolled back patch-by-patch until I found that the first one caused it. Andrew
Re: Wine Wiki needs your help!
On Mon, Jan 14, 2013 at 03:32:40PM -0500, Dimi Paun wrote: > OK, we might be onto something. I've wrote a script > to determine the deleted pages: 20162. > > Should I just go ahead and nuke those? > Probably, yes. One common way for spammers to abuse wikis is to intentionally get the pages deleted, so that they live on permanently in the revision history. See this example from today: It's rightfully deleted as spam: http://wiki.winehq.org/SheriOci But the "Get Info" link leads you to the old revision, with the linkspam intact: http://wiki.winehq.org/SheriOci?action=recall&rev=1 I don't know if there's a way to keep Moinmoin from preserving revision history on deleted pages, but it might be sufficient to simply disable that. I doubt there's much useful history on deleted pages anyway. Andrew
Re: [PATCH] [2/2] kernel32/tests: Add test with a large string for CompareStringA.
On Tue, Jan 08, 2013 at 05:40:44PM +0400, Nikolay Sivov wrote: > On 1/8/2013 16:34, Tatyana Fokina wrote: > > >+ > >+memset(a, 'a', sizeof(a)); > >+SetLastError(NO_ERROR); > >+ret = CompareStringA(lcid, 0, a, sizeof(a), a, sizeof(a)); > >+error = GetLastError(); > >+ok(!error && ret == CSTR_EQUAL, "ret %d (expected %d), error %d\n", > >ret, CSTR_EQUAL, error); > It's better to set it to some value you don't expect to be ever used > as error code, like 0xdeadbeef or 0xb19b00b5. > Maybe avoid that particular sequence of bits ;) <http://lkml.org/lkml/2012/7/18/625> Andrew
Re: [PATCH] dsound: rework ugly mixer logic
On Mon, Dec 31, 2012 at 07:03:31PM +0100, Maarten Lankhorst wrote:
> Op 31-12-12 17:59, Andrew Eikum schreef:
> > On Sat, Dec 29, 2012 at 01:24:03AM +0100, Maarten Lankhorst wrote:
> >> + if(!maxq){
> >> + /* nothing to do! */
> >> + LeaveCriticalSection(&device->mixlock);
> >> + return;
> >>}
> > This was removed in 8ba4090fc304993. It breaks starting the device in
> > some situations (write primary mode iirc).
> Writeprimary dsound tests still worked for me, I don't particulary care
> though, if you want I'll zap it.
> After rework fail it should no longer matter..
>
Zaxxon (see Bug 30836) shows the problem on some, but not all,
systems. Anyway, PerformMix() does more than just mix and write data,
so returning early only because we have no frames to write is not
always correct.
> >> + if (native) {
> >> + void *buffer = NULL;
> >> +
> >> + hr = IAudioRenderClient_GetBuffer(device->render, maxq
> >> / block, (void*)&buffer);
> >> + if(FAILED(hr)){
> >> + WARN("GetBuffer failed: %08x\n", hr);
> >> + LeaveCriticalSection(&device->mixlock);
> >> + return;
> >> + }
> > I think this (mixing directly to RenderClient and skipping WaveQueue)
> > could be split into a separate patch.
> Hate to sound like a broken record here, but the whole mixer logic breaks if
> you touch that code. :/
>
Really? It looks like an optimization to me: mix directly into the
RenderClient buffer instead of to the intermediary buffer if the
target format is float. What breaks if we use the intermediary buffer
in every case?
> >> hres = IAudioClient_Initialize(device->client,
> >> AUDCLNT_SHAREMODE_SHARED, AUDCLNT_STREAMFLAGS_NOPERSIST |
> >> -AUDCLNT_STREAMFLAGS_EVENTCALLBACK, prebuf_rt, 0,
> >> device->pwfx, NULL);
> >> +AUDCLNT_STREAMFLAGS_EVENTCALLBACK, 80, 0, device->pwfx,
> >> NULL);
> > ...
> >> +frames = (UINT64)device->pwfx->nSamplesPerSec * 80 / 1000;
> > Could you #define the 80?
> This is a very temporary solution as that call should not fail, and the
> branch will be killed off in the rework fail patch,
> where it'll be treated like any other error.
>
> As such the only place to get that variable would be
> IAudioClient::Initialize, which I think would be overkill to #define
> somewhere,
> the rest will just use buffer length as returned by the driver, instead of
> making a temporary guess.
>
Okay, fair enough.
Andrew
Re: [PATCH] dsound: rework ugly mixer logic
On Sat, Dec 29, 2012 at 01:24:03AM +0100, Maarten Lankhorst wrote:
> + if(!maxq){
> + /* nothing to do! */
> + LeaveCriticalSection(&device->mixlock);
> + return;
> }
This was removed in 8ba4090fc304993. It breaks starting the device in
some situations (write primary mode iirc).
> + if (native) {
> + void *buffer = NULL;
> +
> + hr = IAudioRenderClient_GetBuffer(device->render, maxq
> / block, (void*)&buffer);
> + if(FAILED(hr)){
> + WARN("GetBuffer failed: %08x\n", hr);
> + LeaveCriticalSection(&device->mixlock);
> + return;
> + }
I think this (mixing directly to RenderClient and skipping WaveQueue)
could be split into a separate patch.
> hres = IAudioClient_Initialize(device->client,
> AUDCLNT_SHAREMODE_SHARED, AUDCLNT_STREAMFLAGS_NOPERSIST |
> -AUDCLNT_STREAMFLAGS_EVENTCALLBACK, prebuf_rt, 0, device->pwfx,
> NULL);
> +AUDCLNT_STREAMFLAGS_EVENTCALLBACK, 80, 0, device->pwfx,
> NULL);
...
> +frames = (UINT64)device->pwfx->nSamplesPerSec * 80 / 1000;
Could you #define the 80?
Anyway, I gave it a test on ALSA+Pulse and didn't find any issues.
I'll test the other backends when you resubmit.
Andrew
Re: oleaut32: Indentation fix (Try 2)
Sorry, I should have labelled this patch as "Try 2". -- Andy.
Re: dsound: use event based threads, v2
Looks good to me, no problems with any of the drivers. I think it'd be
good to get it in on Monday, as release day is tomorrow, but that's
not my call.
Andrew
On Wed, Dec 19, 2012 at 10:30:04AM +0100, Maarten Lankhorst wrote:
> Use a thread instead of a timer for greater precision.
>
> Changes since v1:
> - Add WARN on failure of Start or GetStreamLatency
> - Set lower bound of sleep time to 5 ms
> - Change upper bound of sleep time to 2.5x period
> - Fixup comment
>
> Signed-off-by: Maarten Lankhorst
> ---
> I like how the most useful part of the changelog always gets stripped out.
> What's the point of even adding this info here?
>
> Obligatory "Yes I know the error handling I added isn't perfect, and I did
> fix that in my failure rework patch" here..
>
> ---
> dlls/dsound/dsound.c | 21 +++--
> dlls/dsound/dsound_private.h | 7 ---
> dlls/dsound/mixer.c | 39 +++
> dlls/dsound/primary.c| 27 ---
> 4 files changed, 62 insertions(+), 32 deletions(-)
>
> diff --git a/dlls/dsound/dsound.c b/dlls/dsound/dsound.c
> index ac85ba2..381d0e1 100644
> --- a/dlls/dsound/dsound.c
> +++ b/dlls/dsound/dsound.c
> @@ -670,14 +670,13 @@ ULONG DirectSoundDevice_Release(DirectSoundDevice *
> device)
> TRACE("(%p) ref was %u\n", device, ref + 1);
> if (!ref) {
> int i;
> -timeKillEvent(device->timerID);
> -timeEndPeriod(DS_TIME_RES);
>
> -/* The kill event should have allowed the timer process to expire
> - * but try to grab the lock just in case. Can't hold lock because
> - * secondarybuffer_destroy also grabs the lock */
> -RtlAcquireResourceShared(&(device->buffer_list_lock), TRUE);
> -RtlReleaseResource(&(device->buffer_list_lock));
> +SetEvent(device->sleepev);
> +if (device->thread) {
> +WaitForSingleObject(device->thread, INFINITE);
> +CloseHandle(device->thread);
> +}
> +CloseHandle(device->sleepev);
>
> EnterCriticalSection(&DSOUND_renderers_lock);
> list_remove(&device->entry);
> @@ -813,6 +812,7 @@ HRESULT DirectSoundDevice_Initialize(DirectSoundDevice **
> ppDevice, LPCGUID lpcG
>
> device->mmdevice = mmdevice;
> device->guid = devGUID;
> +device->sleepev = CreateEventW(0, 0, 0, 0);
>
> hr = DSOUND_ReopenDevice(device, FALSE);
> if (FAILED(hr))
> @@ -869,9 +869,10 @@ HRESULT DirectSoundDevice_Initialize(DirectSoundDevice
> ** ppDevice, LPCGUID lpcG
> ZeroMemory(&device->volpan, sizeof(device->volpan));
>
> hr = DSOUND_PrimaryCreate(device);
> -if (hr == DS_OK)
> -device->timerID = DSOUND_create_timer(DSOUND_timer,
> (DWORD_PTR)device);
> -else
> +if (hr == DS_OK) {
> +device->thread = CreateThread(0, 0, DSOUND_mixthread, device, 0, 0);
> +SetThreadPriority(device->thread, THREAD_PRIORITY_TIME_CRITICAL);
> +} else
> WARN("DSOUND_PrimaryCreate failed: %08x\n", hr);
>
> *ppDevice = device;
> diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h
> index 62656a5..1a3900c 100644
> --- a/dlls/dsound/dsound_private.h
> +++ b/dlls/dsound/dsound_private.h
> @@ -68,9 +68,9 @@ struct DirectSoundDevice
>
> GUIDguid;
> DSCAPS drvcaps;
> -DWORD priolevel;
> +DWORD priolevel, sleeptime;
> PWAVEFORMATEX pwfx, primary_pwfx;
> -UINTtimerID, playing_offs_bytes, in_mmdev_bytes,
> prebuf;
> +UINTplaying_offs_bytes, in_mmdev_bytes, prebuf;
> DWORD fraglen;
> LPBYTE buffer;
> DWORD writelead, buflen, state, playpos, mixpos;
> @@ -97,6 +97,7 @@ struct DirectSoundDevice
> IAudioStreamVolume *volume;
> IAudioRenderClient *render;
>
> +HANDLE sleepev, thread;
> struct list entry;
> };
>
> @@ -226,7 +227,7 @@ void DSOUND_AmpFactorToVolPan(PDSVOLUMEPAN volpan)
> DECLSPEC_HIDDEN;
> void DSOUND_RecalcFormat(IDirectSoundBufferImpl *dsb) DECLSPEC_HIDDEN;
> DWORD DSOUND_secpos_to_bufpos(const IDirectSoundBufferImpl *dsb, DWORD
> secpos, DWORD secmixpos, float *overshot) DECLSPEC_HIDDEN;
>
> -void CALLBACK DSOUND_timer(UINT timerID, UINT msg, DWORD_PTR dwUser,
> DWORD_PTR dw1, DWORD_PTR dw2) DECLSPEC_HIDDEN;
> +D
Re: mmdevapi: Avoid lock contention after SetEvent.
On Thu, Dec 20, 2012 at 02:03:06PM +0100, joerg-cyril.hoe...@t-systems.com wrote: > what's up with this patch? > > I wrote: > >SetEvent is one of those calls with a high probability that the > >Linux scheduler will switch to another thread, e.g. the one receiving > >the event. As the critical section is still hold, it'll prevent the > >receiver from invoking mmdevapi, e.g. GetCurrentPadding, without two > >more thread switches (forth and back). We don't need that. > > Now that This->event is constant between IAC_Start and IAC_Release, > thanks to my "SetEventHandle is allowed once only" patch, > it's safe to move SetEvent(This->event) outside of the CS. > > Unlike the rejected patch about a CreateTimerQueue race condition > internal to Wine (and only relevant in lock-less mode), > this one is between the driver and the client app that blocks when calling > e.g. > GetCurrentPadding, what almost all apps will do, including our DSound > http://source.winehq.org/source/dlls/dsound/mixer.c#L679 > I don't have any problems with this patch, but I don't know our synchronization methods well enough to know if this is correct. Is it true that SetEvent() causes a context switch? I see it eventually resolves down to a write() in , but I don't know what that implies. Julliard, any thoughts? Andrew
Re: dsound: change mix rate of primary buffer
On Tue, Dec 18, 2012 at 08:08:03PM +0100, sebastien chev wrote: > Thank you for the tips. I will look at these. > I guess you're probably also thinking about the mix buffer in dsound itself. That does always mix to float, and there isn't a way to change it right now. What are you planning to use this for? We could support different mix buffer formats, but I'm not sure this use case is worth the effort. Andrew
Re: winealsa: Separate read and write pointers.
On Mon, Dec 17, 2012 at 05:01:29PM +0100, joerg-cyril.hoe...@t-systems.com wrote: > Furthermore, this patch eases integration of my lock-less code, which > I'll not release for now, by cleanly separating read and write > pointers during playback and capture. No more adding of two volatile > entities moving in opposite directions! > This seems to be the main goal of the patch. I tested it against my usual set of applications both with and without PulseAudio. This patch seems fine to me. I'll take a look at your lockless patch on bug 29531, and maybe we can get that into Wine after the next release? Andrew
Re: dsound: change mix rate of primary buffer
On Sun, Dec 16, 2012 at 11:56:46AM +0100, sebastien chev wrote: > It seems that no matter what the Windows side program choose for > DirectSoundCreate, the DSound subsystem initialize itself with the higher > spec sound available (for example 48Khz Float32, or 48kHz Int16 if no > Float32 available). > Maarten very recently updated dsound's behavior. It will either choose the default format for the backend or the nearest supported format to the format the application requests. See DSOUND_WaveFormat in . > While not a big deal on current linux PC, using this on ARM device kill > performance. > > A DirectSound subsystem for ARM, with Integer only for the mixbuffer (as > most ARM still use softfloat) and 22kHz should be less taxing for > performances. > It seems like the best solution is for the backend to prefer a format that the CPU supports efficiently. This requires some more intelligence in AudioClient_GetMixFormat in , which currently probes for capabilities and chooses the best available format. Andrew
Re: gdi32: Indentation fix
Dmitry Timoshkov wrote: > Andrew Talbot wrote: > >> Changelog: >> gdi32: Indentation fix. > > Please keep 4 spaces indentation without tabs. > Thus far, I have fixed the bits of code that are misleadingly indented using the same indentation regime as the surrounding code, primarily to make it as clear as I can precisely which lines were wrong in the first place. Should I be reformatting the whole of each function I fix to bring it in line with the 4-space standard, instead?
Re: RFC: Remove auto-scan of ALSA devices from winealsa.drv
On Wed, Dec 12, 2012 at 04:45:11PM +0100, Henri Verbeet wrote: > On 12 December 2012 16:31, Andrew Eikum wrote: > > Even ignoring the Pulse case, we don't have an acceptable enumeration > > API. > Yes, I know. I just don't think it would be unreasonable to try to > work with ALSA upstream to fix that. I asked about this topic back in January and didn't really receive a useful response: http://mailman.alsa-project.org/pipermail/alsa-devel/2012-January/047821.html In fact, Jörg's original question was basically ignored: http://thread.gmane.org/gmane.linux.alsa.devel/92878/focus=92923 There doesn't seem to be much motivation to fix this. I know the ALSA devs are working on a channel mapping API, which will (probably?) make the surroundXX interfaces obsolete. I've been trying to think of a way to define "useful ALSA interface," and I'm coming up blank. Should it just be all of "plug:'CARD=ABC,DEV=n'" and all interfaces in various asoundrc? Do we wait until the channel mapping API is in so we can obsolete the surroundXX stuff? What about other plugins like "dmix"? Are those useful to enumerate? Is there a way to enumerate plugins? I think the problem is complicated enough and unimportant enough that it's not really on anyone's plate. Meanwhile Wine displays useless devices. Andrew
Re: RFC: Remove auto-scan of ALSA devices from winealsa.drv
On Wed, Dec 12, 2012 at 03:57:40PM +0100, Henri Verbeet wrote: > On 12 December 2012 15:28, Andrew Eikum wrote: > > It's tricky because ALSA and PulseAudio have different theories about > > where device selection should occur -- in the application or in the > > audio mixer. In the ALSA case, we want to list devices. In the Pulse > > case, we want to list only "default". > > > But supposedly we'll have a winepulse driver eventually, so winealsa > would only need to care about actual ALSA devices. Even ignoring the Pulse case, we don't have an acceptable enumeration API. It doesn't seem unreasonable to require users with strange audio setups to tell Wine what to do. We could provide some convenience features, like a list of suggested devices that the user would have to explicitly enable. Andrew
Re: RFC: Remove auto-scan of ALSA devices from winealsa.drv
On Wed, Dec 12, 2012 at 04:23:52PM +0100, joerg-cyril.hoe...@t-systems.com wrote: > Max TenEyck Woodbury objected: > >Replacing the ability to use a drop box in the app to select the audio > >device with a 'regedit' session is not an improvement. > My proposal intentionaly was silent about GUI changes, but you are right: > in such a case, winecfg should provide a textedit box for users to enter a > (or edit the registry's) comma-separated list of ALSA device names. > I'm not sure if winecfg is currently sufficiently detached from winmm/mmdevapi > for such editing to take effect immediately. I remember bugs where the > "Test Sound" button would continue to use the previously selected device. > That stuff requires IMMNotificationClient, which I actually do have an implementation of, but I'm not quite happy with it yet. I don't think lacking that interface prevents us from doing this particular enhancement, though. Andrew
Re: RFC: Remove auto-scan of ALSA devices from winealsa.drv
On Tue, Dec 11, 2012 at 04:46:34PM +0100, Henri Verbeet wrote: > On 11 December 2012 16:05, wrote: My first reaction is that this is a good idea. We've had some discussion about device enumeration before. The final conclusion was it's basically impossible to do usefully with ALSA. I can't think of a way for us or for ALSA to enumerate only "useful" devices. The closest that currently exists is snd_device_name_hint, but that has plenty of problems, too: http://www.winehq.org/pipermail/wine-devel/2012-February/094182.html For comparison, here's some examples of what various libraries and applications do: snd_device_name_hint: aplay, VLC Hard-coded based on number of channels: mplayer, SDL snd_card_next: Wine, gstreamer There is clearly no consensus. > > Cost to users: > > > > Users with a working ALSA device "default" should experience no > > drawback, only benefits. I believe this is the vast majority of users. > > > > Users that edit their ~/.asoundrc to define other devices without > > simultaneously overriding !default will have to additionally edit the > > Wine registry to name their working ALSA capture and render devices, > > e.g. "asnoop" and "amix". > > > It will also pretty much just remove device selection on setup with > multiple audio devices, which is actually fairly common these days > with USB audio devices and HDMI outputs on graphics cards. I think the > correct approach would to work with upstream ALSA to fix things, > instead of just removing device enumeration. > It's tricky because ALSA and PulseAudio have different theories about where device selection should occur -- in the application or in the audio mixer. In the ALSA case, we want to list devices. In the Pulse case, we want to list only "default". The fact that ALSA still doesn't have a usable enumeration API makes me think that enumeration is not an intended use-case. The official ALSA sample applications tend to use "default" or allow the user to specify their own device. That seems like a fine compromise. So it seems reasonable to me to list only "default", but also provide an easy way to add new devices. Forcing users like Max to go to regedit in every new prefix to add a new audio device really sucks. This means some sort of driver-specific dialog in winecfg (or control panel?). That sucks too, but I think it's the least-bad solution. Andrew
Re: [PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound
On Thu, Dec 06, 2012 at 12:14:53AM +0800, Dmitry Timoshkov wrote: > Andrew Eikum wrote: > > > > > The call is only supposed to return TRUE or FALSE, so I guess it's > > > > checking that the return value isn't 3 or something. Not very useful, > > > > sure, but it looked odd to just have a series of sndPlaySound calls in > > > > a row without ok() calls. The real test is to make sure sndPlaySound > > > > doesn't crash, as it does without my patch. > > > > > > Then the test is completely useless, it actually tests nothing. > > > > > > > The ok calls are useless, I guess, but they're not doing any harm, > > either. You can send a patch to remove the ok calls if you like. > > Without an ok() call a test is meaningless, I guess that Alexandre committed > it by an accident. Since it's your patch I'll leave a revert to you. > There is an implied test which is that the tests do not crash. Without the change to , the tests you object to will crash. With the change, they do not crash. This is the behavior being tested. Andrew
Re: [PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound
On Wed, Dec 05, 2012 at 11:21:39PM +0800, Dmitry Timoshkov wrote: > Andrew Eikum wrote: > > The call is only supposed to return TRUE or FALSE, so I guess it's > > checking that the return value isn't 3 or something. Not very useful, > > sure, but it looked odd to just have a series of sndPlaySound calls in > > a row without ok() calls. The real test is to make sure sndPlaySound > > doesn't crash, as it does without my patch. > > Then the test is completely useless, it actually tests nothing. > The ok calls are useless, I guess, but they're not doing any harm, either. You can send a patch to remove the ok calls if you like. Andrew
Re: [PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound
On Wed, Dec 05, 2012 at 11:06:48PM +0800, Dmitry Timoshkov wrote: > Andrew Eikum wrote: > > > > > +br = sndPlaySoundA((LPCSTR)SND_ALIAS_SYSTEMASTERISK, > > > > SND_ALIAS_ID|SND_SYNC); > > > > +ok(br == TRUE || br == FALSE, "sndPlaySound gave strange return: > > > > %u\n", br); > > > > > > This kind of test is broken. > > > > > > > Care to elaborate? Being this terse helps no one. > > What is this code testing the return value for? Why are there all these casts? > The call is only supposed to return TRUE or FALSE, so I guess it's checking that the return value isn't 3 or something. Not very useful, sure, but it looked odd to just have a series of sndPlaySound calls in a row without ok() calls. The real test is to make sure sndPlaySound doesn't crash, as it does without my patch. The casts are to avoid compilation warnings. sndPlaySound takes a string as its first parameter, but it also accepts symbols like SND_ALIAS_SYSTEMASTERISK, which has the value 0x2A53. A cast is needed to avoid the warning about converting an integer to a pointer. Andrew
Re: [PATCH] winmm: Don't mask out SND_ALIAS_ID or SND_FILENAME in sndPlaySound
On Wed, Dec 05, 2012 at 11:59:01AM +0800, Dmitry Timoshkov wrote: > Andrew Eikum wrote: > > > +br = sndPlaySoundA((LPCSTR)SND_ALIAS_SYSTEMASTERISK, > > SND_ALIAS_ID|SND_SYNC); > > +ok(br == TRUE || br == FALSE, "sndPlaySound gave strange return: > > %u\n", br); > > This kind of test is broken. > Care to elaborate? Being this terse helps no one. Andrew
Re: [PATCH] dsound: Use event based threads
On Mon, Dec 03, 2012 at 02:59:07PM +0100, joerg-cyril.hoe...@t-systems.com wrote: > BTW, I still believe that mixing and resampling would find their best place > in mmdevapi, not DSound. > Perhaps, but that's provably not what Windows does. Given a format with: dwChannelMask = (CHANNEL_BACK_LEFT | CHANNEL_BACK_RIGHT) and a physical speaker setup containing only: (CHANNEL_FRONT_LEFT | CHANNEL_FRONT_RIGHT) dsound and mmdevapi behave differently. mmdevapi maps the BACK_X channel entirely out of the respective FRONT_X speaker. dsound splits about 2/3 of the matching channel, and 1/3 out of the other FRONT channel, so you hear both signals out of both speakers. I think the best solution is to have dsound call into the system openal to perform multichannel mixing and 3D processing. We can start by getting dsound to have rudimentary support for those features, then plug openal in as the last step. Andrew
Re: [PATCH] dsound: Use event based threads
On Thu, Nov 29, 2012 at 12:50:20PM +0100, Maarten Lankhorst wrote: > Op 21-11-12 19:16, Maarten Lankhorst schreef: > > Signed-off-by: Maarten Lankhorst > > > Bump, anything wrong with this patch? > Been spending my "Wine time" on Bug 32297 this week. I'll take a look at this shortly. Andrew
Re: Explicit fall-through in switch statements?
Perhaps I should add that the list is of caseS/defaultS that may be fallen through to, rather than out from. Unfortunately,because I produced it in a hurry, it does contain the odd copy-and-paste error for case names (e.g., the case for dmusic/collection.c line 409 should be default:, not case 8:, and that for dlls/oledb32/convert.c line 873 should be case DBTYPE_NUMERIC: not case VT_BSTR:), but I think the line numbers are more trustworthy. -- Andy.
Re: Explicit fall-through in switch statements?
Vincent Povirk wrote: > It might be useful to post a listing of the files where unmarked > fall-throughs (falls-through?) appear, and I could see if any of them > are on my turf. Here is a rough-and-ready list of where they are. dlls/msvcp100/exception.c (line 498) case EXCEPTION: dlls/msvcp100/exception.c (line 503) case EXCEPTION_BAD_ALLOC: dlls/msvcp100/exception.c (line 508) case EXCEPTION_LOGIC_ERROR: dlls/msvcp100/exception.c (line 513) case EXCEPTION_LENGTH_ERROR: dlls/msvcp100/exception.c (line 518) case EXCEPTION_OUT_OF_RANGE: dlls/msvcp100/exception.c (line 523) case EXCEPTION_INVALID_ARGUMENT: dlls/msvcp100/exception.c (line 528) case EXCEPTION_RUNTIME_ERROR: dlls/msvcp100/exception.c (line 533) case EXCEPTION_FAILURE: dlls/comctl32/listview.c (line 5875) default: dlls/comctl32/monthcal.c (line 2059) default: dlls/comctl32/tab.c (line 3465) case WM_SETFOCUS: dlls/comctl32/toolbar.c (line 2307) case DL_CANCELDRAG: dlls/comctl32/treeview.c (line 4113) case TVHT_ONITEMLABEL: dlls/d3d10core/view.c (line 136) case D3D10_RESOURCE_DIMENSION_BUFFER: dlls/d3d10core/view.c (line 375) case D3D10_RESOURCE_DIMENSION_BUFFER: dlls/ddraw/utils.c (line 450) default: dlls/ddraw/utils.c (line 464) case 8: dlls/dmusic/collection.c (line 409) case 8: dlls/dplayx/dplay.c (line 708) case DPMSGCMD_FORWARDADDPLAYER: dlls/gdi32/bidi.c (line 515) default: dlls/gdi32/painting.c (line 161) default: dlls/gdiplus/graphics.c (line 962) default: dlls/gdiplus/graphicspath.c (line 1756) default: dlls/gphoto2.ds/gphoto2_main.c (line 1023) default: dlls/inetcomm/pop3transport.c (line 129) default: dlls/inetcomm/pop3transport.c (line 154) default: dlls/inetcomm/pop3transport.c (line 200) default: dlls/jscript/dispex.c (line 457) case PROP_PROTREF: dlls/jscript/regexp.c (line 1680) case PROP_PROTREF: dlls/jscript/regexp.c (line 1941) case PROP_PROTREF: dlls/kernel32/volume.c (line 786) case FS_FAT32: dlls/mshtml/htmlevent.c (line 1093) case FS_FAT32: dlls/mshtml/script.c (line 211) case FS_FAT32: dlls/mshtml/script.c (line 216) case FS_FAT32: dlls/msvcp90/exception.c (line 498) case FS_FAT32: dlls/msvcp90/exception.c (line 503) case FS_FAT32: dlls/msvcp90/exception.c (line 508) case FS_FAT32: dlls/msvcp90/exception.c (line 513) case FS_FAT32: dlls/msvcp90/exception.c (line 518) case FS_FAT32: dlls/msvcp90/exception.c (line 523) case FS_FAT32: dlls/msvcp90/exception.c (line 528) case FS_FAT32: dlls/msvcp90/exception.c (line 533) case FS_FAT32: dlls/msvcrtd/debug.c (line 61) case FS_FAT32: dlls/netapi32/access.c (line 950) case FS_FAT32: dlls/ntdll/reg.c (line 943) case FS_FAT32: dlls/oleaut32/variant.c (line 4424) case VT_NULL: dlls/oleaut32/variant.c (line 5512) case VT_NULL: dlls/oleaut32/variant.c (line 5519) case VT_NULL: dlls/oleaut32/variant.c (line 5533) case VT_NULL: dlls/oleaut32/variant.c (line 5542) case VT_BSTR: dlls/oledb32/convert.c (line 873) case VT_BSTR: dlls/oledlg/insobjdlg.c (line 199) case IDC_OBJTYPELIST: dlls/rsaenh/rsaenh.c (line 3780) case ERROR_SUCCESS: dlls/sane.ds/ui.c (line 1013) case WM_COMMAND: dlls/serialui/confdlg.c (line 405) default: dlls/setupapi/queue.c (line 243) case SPFILENOTIFY_STARTQUEUE: dlls/shell32/shelldispatch.c (line 709) default: dlls/shell32/shellole.c (line 696) case OPEN_ALWAYS: dlls/shell32/shellole.c (line 702) case OPEN_EXISTING: dlls/shell32/shlmenu.c (line 1226) default: dlls/shlwapi/reg.c (line 730) default: dlls/shlwapi/reg.c (line 790) default: dlls/urlmon/axinstall.c (line 258) case WM_TIMER: dlls/urlmon/internet.c (line 789) default: dlls/user32/edit.c (line 3163) case '\n': dlls/user32/mdi.c (line 1329) case SC_SIZE: dlls/user32/spy.c (line 2463) case WM_WINDOWPOSCHANGING: dlls/user32/spy.c (line 2473) case WM_STYLECHANGING: dlls/user32/spy.c (line 2523) default: dlls/usp10/bidi.c (line 773) default: dlls/usp10/breaking.c (line 131) case b_LF: dlls/wined3d/glsl_shader.c (line 467) case HEAP_NODE_TRAVERSE_RIGHT: dlls/wined3d/glsl_shader.c (line 483) case HEAP_NODE_POP: dlls/wined3d/glsl_shader.c (line 541) case HEAP_NODE_TRAVERSE_RIGHT: dlls/wined3d/glsl_shader.c (line 556) case HEAP_NODE_POP: dlls/wined3d/glsl_shader.c (line 5129) case WINED3D_TOP_BUMPENVMAP: dlls/wined3d/glsl_shader.c (line 5131) case WINED3D_TOP_BLEND_TEXTURE_ALPHA: dlls/wined3d/nvidia_texture_shader.c (line 466) default: dlls/wined3d/state.c (line 2632) default: dlls/wined3d/state.c (line 3093) default: dlls/wined3d/surface.c (line 1183) case WINED3D_TEXF_NONE: dlls/wined3d/surface.c (line 3022) case WINED3D_CONTAINER_NONE: dlls/wined3d/surface.c (line 3050) case WINED3D_CONTAINER_NONE: dlls/winex11.drv/palette.c (line 149) case GrayScale: dlls/winex11.drv/palette.c (line 178) case StaticColor: dlls/winex11.drv/xrender.c (line 1115) case AA_None: dlls/wininet/internet.c (line 3485) case INTERNET_SCHEME_GOPHER: dlls/msvcp60/exception.c (line 613) case EXCEPTION: dlls/msvcp60/exception.c (line 618) cas
Re: msvcp60: Remove superfluous semicolons
Jacek Caban wrote: > It's probably better to change the macro to require the semicolon. > > Jacek The reason I did it that way was because there are two variants of the DEFINE_CXX_DATA macro, surrounded by an #ifndef construct: one comprising three struct declarations, all ending in semicolons, the other comprised three struct declarations (ending in semicolons) and one function definition (ending in a curly brace). I suppose I could move the function above the structs, but that would require forward declarations. -- Andy.
Re: Explicit fall-through in switch statements?
Frédéric Delanoy wrote: > For every wine version, static checkers (like coverity) detect cases > where a switch case automatically falls-through to the next case. > > Shouldn't be there a rule that such cases are always marked with a > "fall-through comment"? > With the possible exception of case with no statement? like 'case FOO: > case BAR: do_something();...'? > > This would probably help IMHO, especially when people refactor old code. > > Frédéric I reckon there are currently about 105 unmarked fall-throughs in the dlls. Unless people consider it offensive, one option might be to make careful use of git blame to find out who wrote the particular cases and ask them whether their fall-throughs were deliberate, marking up or fixing with breakS as appropriate. -- Andy.
Re: [PATCH v4] dsound: create a primary_pwfx separately from pwfx
Yeah, that fixes the problem. This patch looks good to me now.
Andrew
On Fri, Nov 16, 2012 at 08:35:51PM +0100, Maarten Lankhorst wrote:
> I promised I would never touch it, but then I wanted to play skyrim
>
> V2: Limit channels to 2 in primary if not in writeprimary mode
> V3: Split off DSOUND_WaveFormat to its own function sooner.
> V4: Fix accidental revert of device->buflen increase. It was supposed to wait
> until rework in patch 3.
> ---
> diff --git a/dlls/dsound/dsound.c b/dlls/dsound/dsound.c
> index e5b0e3c..ac85ba2 100644
> --- a/dlls/dsound/dsound.c
> +++ b/dlls/dsound/dsound.c
> @@ -305,21 +305,36 @@ static HRESULT WINAPI
> IDirectSound8Impl_SetCooperativeLevel(IDirectSound8 *iface
> DWORD level)
> {
> IDirectSoundImpl *This = impl_from_IDirectSound8(iface);
> +DirectSoundDevice *device = This->device;
> +DWORD oldlevel;
> +HRESULT hr = S_OK;
>
> TRACE("(%p,%p,%s)\n", This, hwnd, dumpCooperativeLevel(level));
>
> -if (!This->device) {
> +if (!device) {
> WARN("not initialized\n");
> return DSERR_UNINITIALIZED;
> }
>
> if (level == DSSCL_PRIORITY || level == DSSCL_EXCLUSIVE) {
> WARN("level=%s not fully supported\n",
> -level == DSSCL_PRIORITY ? "DSSCL_PRIORITY" :
> "DSSCL_EXCLUSIVE");
> + level==DSSCL_PRIORITY ? "DSSCL_PRIORITY" : "DSSCL_EXCLUSIVE");
> }
>
> -This->device->priolevel = level;
> -return DS_OK;
> +RtlAcquireResourceExclusive(&device->buffer_list_lock, TRUE);
> +EnterCriticalSection(&device->mixlock);
> +oldlevel = device->priolevel;
> +device->priolevel = level;
> +if ((level == DSSCL_WRITEPRIMARY) != (oldlevel == DSSCL_WRITEPRIMARY)) {
> +hr = DSOUND_ReopenDevice(device, level == DSSCL_WRITEPRIMARY);
> +if (FAILED(hr))
> +device->priolevel = oldlevel;
> +else
> +DSOUND_PrimaryOpen(device);
> +}
> +LeaveCriticalSection(&device->mixlock);
> +RtlReleaseResource(&device->buffer_list_lock);
> +return hr;
> }
>
> static HRESULT WINAPI IDirectSound8Impl_Compact(IDirectSound8 *iface)
> @@ -612,20 +627,24 @@ static HRESULT
> DirectSoundDevice_Create(DirectSoundDevice ** ppDevice)
> device->guid = GUID_NULL;
>
> /* Set default wave format (may need it for waveOutOpen) */
> -device->pwfx =
> HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,sizeof(WAVEFORMATEX));
> -if (device->pwfx == NULL) {
> +device->pwfx =
> HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,sizeof(WAVEFORMATEXTENSIBLE));
> +device->primary_pwfx =
> HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,sizeof(WAVEFORMATEXTENSIBLE));
> +if (!device->pwfx || !device->primary_pwfx) {
> WARN("out of memory\n");
> +HeapFree(GetProcessHeap(),0,device->primary_pwfx);
> +HeapFree(GetProcessHeap(),0,device->pwfx);
> HeapFree(GetProcessHeap(),0,device);
> return DSERR_OUTOFMEMORY;
> }
>
> device->pwfx->wFormatTag = WAVE_FORMAT_PCM;
> -device->pwfx->nSamplesPerSec = ds_default_sample_rate;
> -device->pwfx->wBitsPerSample = ds_default_bits_per_sample;
> +device->pwfx->nSamplesPerSec = 22050;
> +device->pwfx->wBitsPerSample = 8;
> device->pwfx->nChannels = 2;
> device->pwfx->nBlockAlign = device->pwfx->wBitsPerSample *
> device->pwfx->nChannels / 8;
> device->pwfx->nAvgBytesPerSec = device->pwfx->nSamplesPerSec *
> device->pwfx->nBlockAlign;
> device->pwfx->cbSize = 0;
> +memcpy(device->primary_pwfx, device->pwfx, sizeof(*device->pwfx));
>
> InitializeCriticalSection(&(device->mixlock));
> device->mixlock.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ":
> DirectSoundDevice.mixlock");
> diff --git a/dlls/dsound/dsound_main.c b/dlls/dsound/dsound_main.c
> index 3fded64..3c47423 100644
> --- a/dlls/dsound/dsound_main.c
> +++ b/dlls/dsound/dsound_main.c
> @@ -93,8 +93,6 @@ WCHAR wine_vxd_drv[] = {
> 'w','i','n','e','m','m','.','v','x','d', 0 };
> /* All default settings, you most likely don't want to touch these, see wiki
> on UsefulRegistryKeys */
> int ds_hel_buflen = 32768 * 2;
> int ds_snd_queue_max = 10;
> -int ds_default_sample_rate = 44100;
> -int ds_default_bits_per_sample = 16;
> static HINSTANCE instance;
&
Re: mciavi: Fix player deadlock when starting to play.
Seems reasonable to me. Thanks for working on this old code. On Thu, Nov 08, 2012 at 10:51:08AM +0100, joerg-cyril.hoe...@t-systems.com wrote: >
Re: [PATCH 3/3] winmm: Call IsFormatSupported for WAVE_FORMAT_QUERY only.
Ran tests on all four backends and everything seems okay. On Fri, Oct 26, 2012 at 10:31:33AM +0200, joerg-cyril.hoe...@t-systems.com wrote: > Hi, > > With the previous code in place, we can derive sane values for > WAVE_FORMAT_QUERY. > > Given the amount of work and side-effects that the audio backends perform, > better not > open them twice with IsFormatSupported then Initialize. Just use one or the > other. > > Regards, > Jörg Höhle >
Re: [PATCH 3/6] dsound: create a primary_pwfx separately from pwfx
On Sat, Oct 20, 2012 at 12:13:08AM +0200, Maarten Lankhorst wrote: > Hey, > > Op 19-10-12 15:29, Andrew Eikum schreef: > > Patches 1 and 2 in this series look fine. > > > > I have a series of patches similar to this one in my dsound > > multichannel branch. This patch seems to do too much at once. > > > > When I did this cleanup, I split it up into four patches: > > 1) Allocate the device format in the Device struct > > 2) Load the default format from the IAC, not hard-coded in dsound > > 3) Put SetFormat() calls into their own WFX that we don't really care > > about > > 4) Always use WAVE_FORMAT_EXTENSIBLE for the device format > 3 of 6 does essentially the step you call #4 and #2, other steps is just > fixes I already have in my tree. > Tested quite extensively too > > The other steps I have already done in a followup series, except #1, it's > unneeded. > It also breaks device->pwfx = NULL in the initial case, which is nice to see > if something > depends on the new format before changing over is complete. This would > otherwise > be very hardto detect. > > I'm surprised you call #2 a separate step since from what I gathered about > mmdevapi is that until win7 sp1 it only supported 1 rate, and we don't > implement the rate changing extension yet, so the whole popular format > testing was pointless to begin with. > Like the fraglen change in patch 5, It's a change in dsound's behavior that can be isolated from the rest of the patch. Being able to bisect these kinds of changes is really important, as you can run into some really unexpected bugs from how apps use dsound. Bug 29127 is an example of a regression that really surprised me. > Is there anything *technically* wrong with the patch series though? > I haven't dug into them that far yet. Patch 3 needs work, so I don't want to test the further ones until it's fixed. You don't have to do it the same way I did, but please at least split out the format initialization change from the SetFormat change. Andrew
Re: [PATCH 5/6] dsound: rework ugly mixer logic
On Sat, Oct 20, 2012 at 12:03:55AM +0200, Maarten Lankhorst wrote: > Op 19-10-12 15:54, Andrew Eikum schreef: > > Mostly good cleanup in this one. Some thoughts below... > > > > On Tue, Oct 16, 2012 at 02:06:29PM +0200, Maarten Lankhorst wrote: > >> diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h > >> index feef787..7817b88 100644 > >> --- a/dlls/dsound/dsound_private.h > >> +++ b/dlls/dsound/dsound_private.h > >> @@ -73,10 +73,8 @@ struct DirectSoundDevice > >> -DWORD writelead, buflen, state, playpos, mixpos; > >> +DWORD writelead, buflen, aclen, fraglen, state, > >> playpos, pad; > > If you're introducing new variables, surely you can come up with > > something more descriptive than "aclen." Something like > > "ac_buffer_frames," maybe? I'm a big fan of units on my variable > > names so mistakes like "pos_bytes = ac_frames;" are obvious. > > > > Similar for "pad," maybe something like "in_mmdev_bytes" (Is that > > actually used any more after your patches? You could re-use it if > > not.) > However what I do makes sense here, since all of the units in dsound right > now are bytes, unless that gets changed. So I stick to them. > I'm all for changing it to frames, but it was out of the scope for this patch. > Fair enough, I hadn't thought of that. I still think "aclen" is a crummy name :P > And.. get out of my bikeshed! > Well, it does lead to errors (bbbf72ddcb1202) and I don't see any harm in being explicit. It'll future-proof it, in case future variables have some other unit. In any case, not a patch-killer, of course. > >> + block = device->pwfx->nBlockAlign; > > Bleh. Do we really need to alias that? > It's my bikeshed to paint! :P > My objection is it makes the code harder to read, which is something we really don't need in dsound. For an even worse example, see the w/wi/wfe/wfe2/wfe3 web in your 3rd patch. > Followup patch I didn't send in yet on removed this entirely and just > consolidated both cases. > > > > >> + if (maxq > device->fraglen * 3) > >> + maxq = device->fraglen * 3; > >> + > > This seems to be replacing ds_snd_queue_max, right? Why remove the > > configurability? Why 3 instead of the old default of 10? This should > > be a separate patch at least. > No this does not replace ds_snd_queue_max, I always fill up the buffer to > full, > but I'm supposed to get a event every period, so instead of always filling the > buffer to full I spread it out over multiple periods so hopefully applications > have some time to catch up. This gets rid of clipping at the start. > Okay, thanks for explaining. I was going to give the behavior changes a closer look after we figure out patch 3 and I get a chance to run my tests. Andrew
Re: [PATCH 4/6] dsound: Use event based threads
On Fri, Oct 19, 2012 at 11:33:22PM +0200, Maarten Lankhorst wrote: > Op 19-10-12 15:40, Andrew Eikum schreef: > > I'm a big fan of error checking (or at least reporting), so these > > unchecked calls make me nervous. They'll probably never fail, but if > > they do, I think they should fail loudly. > > > Erm, dsound at this point will be worse off if you perform error reporting > here than fail silently. > It leaves a bigger mess since ReopenDevice is not atomic right now.. > What I meant by "reporting" was write a WARN or something. Then we know something wrong happened even if we don't have the correct driver channel enabled. Andrew
Re: [PATCH 6/6] dsound: remove now unused variables
Be nice if you could split this up and squash it into the previous
patches.
Andrew
On Tue, Oct 16, 2012 at 02:06:30PM +0200, Maarten Lankhorst wrote:
> From: Maarten Lankhorst
>
> No longer influence anything, so zap them.
> ---
> dlls/dsound/dsound_main.c| 17 -
> dlls/dsound/dsound_private.h | 4
> 2 files changed, 21 deletions(-)
>
> diff --git a/dlls/dsound/dsound_main.c b/dlls/dsound/dsound_main.c
> index 3fded64..607c3d4 100644
> --- a/dlls/dsound/dsound_main.c
> +++ b/dlls/dsound/dsound_main.c
> @@ -29,7 +29,6 @@
> * Handle static buffers - put those in hardware, non-static not in
> hardware
> * Hardware DuplicateSoundBuffer
> * Proper volume calculation for 3d buffers
> - * Remove DS_HEL_FRAGS and use mixer fragment length for it
> */
>
> #include
> @@ -92,9 +91,6 @@ WCHAR wine_vxd_drv[] = {
> 'w','i','n','e','m','m','.','v','x','d', 0 };
>
> /* All default settings, you most likely don't want to touch these, see wiki
> on UsefulRegistryKeys */
> int ds_hel_buflen = 32768 * 2;
> -int ds_snd_queue_max = 10;
> -int ds_default_sample_rate = 44100;
> -int ds_default_bits_per_sample = 16;
> static HINSTANCE instance;
>
> /*
> @@ -147,23 +143,10 @@ void setup_dsound_options(void)
> if (!get_config_key( hkey, appkey, "HelBuflen", buffer, MAX_PATH ))
> ds_hel_buflen = atoi(buffer);
>
> -if (!get_config_key( hkey, appkey, "SndQueueMax", buffer, MAX_PATH ))
> -ds_snd_queue_max = atoi(buffer);
> -
> -
> -if (!get_config_key( hkey, appkey, "DefaultSampleRate", buffer, MAX_PATH
> ))
> -ds_default_sample_rate = atoi(buffer);
> -
> -if (!get_config_key( hkey, appkey, "DefaultBitsPerSample", buffer,
> MAX_PATH ))
> -ds_default_bits_per_sample = atoi(buffer);
> -
> if (appkey) RegCloseKey( appkey );
> if (hkey) RegCloseKey( hkey );
>
> TRACE("ds_hel_buflen = %d\n", ds_hel_buflen);
> -TRACE("ds_snd_queue_max = %d\n", ds_snd_queue_max);
> -TRACE("ds_default_sample_rate = %d\n", ds_default_sample_rate);
> -TRACE("ds_default_bits_per_sample = %d\n", ds_default_bits_per_sample);
> }
>
> static const char * get_device_id(LPCGUID pGuid)
> diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h
> index 7817b88..bdd3a37 100644
> --- a/dlls/dsound/dsound_private.h
> +++ b/dlls/dsound/dsound_private.h
> @@ -31,10 +31,6 @@
> #include "wine/list.h"
>
> extern int ds_hel_buflen DECLSPEC_HIDDEN;
> -extern int ds_snd_queue_max DECLSPEC_HIDDEN;
> -extern int ds_snd_shadow_maxsize DECLSPEC_HIDDEN;
> -extern int ds_default_sample_rate DECLSPEC_HIDDEN;
> -extern int ds_default_bits_per_sample DECLSPEC_HIDDEN;
>
>
> /*
> * Predeclare the interface implementation structures
> --
> 1.7.11.3
>
>
>
Re: [PATCH 5/6] dsound: rework ugly mixer logic
Mostly good cleanup in this one. Some thoughts below...
On Tue, Oct 16, 2012 at 02:06:29PM +0200, Maarten Lankhorst wrote:
> diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h
> index feef787..7817b88 100644
> --- a/dlls/dsound/dsound_private.h
> +++ b/dlls/dsound/dsound_private.h
> @@ -73,10 +73,8 @@ struct DirectSoundDevice
> -DWORD writelead, buflen, state, playpos, mixpos;
> +DWORD writelead, buflen, aclen, fraglen, state,
> playpos, pad;
If you're introducing new variables, surely you can come up with
something more descriptive than "aclen." Something like
"ac_buffer_frames," maybe? I'm a big fan of units on my variable
names so mistakes like "pos_bytes = ac_frames;" are obvious.
Similar for "pad," maybe something like "in_mmdev_bytes" (Is that
actually used any more after your patches? You could re-use it if
not.)
> @@ -209,7 +207,6 @@ HRESULT DSOUND_PrimaryCreate(DirectSoundDevice *device)
> DECLSPEC_HIDDEN;
> HRESULT DSOUND_PrimaryDestroy(DirectSoundDevice *device) DECLSPEC_HIDDEN;
> HRESULT DSOUND_PrimaryPlay(DirectSoundDevice *device) DECLSPEC_HIDDEN;
> HRESULT DSOUND_PrimaryStop(DirectSoundDevice *device) DECLSPEC_HIDDEN;
> -HRESULT DSOUND_PrimaryGetPosition(DirectSoundDevice *device, LPDWORD
> playpos, LPDWORD writepos) DECLSPEC_HIDDEN;
Good riddance.
> @@ -682,147 +623,100 @@ static void DSOUND_PerformMix(DirectSoundDevice
> *device)
> LeaveCriticalSection(&device->mixlock);
> return;
> }
> -
> - to_mix_frags = device->prebuf - (pad * device->pwfx->nBlockAlign +
> device->fraglen - 1) / device->fraglen;
> -
> - to_mix_bytes = to_mix_frags * device->fraglen;
> -
> - if(device->in_mmdev_bytes > 0){
> - DWORD delta_bytes = min(to_mix_bytes, device->in_mmdev_bytes);
> - device->in_mmdev_bytes -= delta_bytes;
> - device->playing_offs_bytes += delta_bytes;
> - device->playing_offs_bytes %= device->buflen;
> + block = device->pwfx->nBlockAlign;
Bleh. Do we really need to alias that?
> + if (maxq > device->fraglen * 3)
> + maxq = device->fraglen * 3;
> +
This seems to be replacing ds_snd_queue_max, right? Why remove the
configurability? Why 3 instead of the old default of 10? This should
be a separate patch at least.
> + writepos = (device->playpos + pad) % device->buflen;
>
> if (device->priolevel != DSSCL_WRITEPRIMARY) {
> - BOOL recover = FALSE, all_stopped = FALSE;
> - DWORD playpos, writepos, writelead, maxq, prebuff_max,
> prebuff_left, size1, size2;
> - LPVOID buf1, buf2;
> + BOOL all_stopped = FALSE;
> int nfiller;
> + BOOL native = device->normfunction == normfunctions[4];
> + DWORD bpp = device->pwfx->wBitsPerSample>>3;
Again, do we need to alias that?
> -static DWORD DSOUND_fraglen(DirectSoundDevice *device)
> -{
> -REFERENCE_TIME period;
> -HRESULT hr;
> -DWORD ret;
> -
> -hr = IAudioClient_GetDevicePeriod(device->client, &period, NULL);
> -if(FAILED(hr)){
> -/* just guess at 10ms */
> -WARN("GetDevicePeriod failed: %08x\n", hr);
> -ret = MulDiv(device->pwfx->nBlockAlign,
> device->pwfx->nSamplesPerSec, 100);
> -}else
> -ret = MulDiv(device->pwfx->nSamplesPerSec *
> device->pwfx->nBlockAlign, period, 1000);
> -
> -ret -= ret % device->pwfx->nBlockAlign;
> -return ret;
> -}
> -
...
> +device->fraglen = MulDiv(device->pwfx->nSamplesPerSec, period, 1000)
> * device->pwfx->nBlockAlign;
This should be a separate patch. I don't have an argument /against/
it, but why do you prefer 10ms over whatever the driver prefers?
Andrew
Re: [PATCH 4/6] dsound: Use event based threads
I like it. Before I can give a sign-off, I need to run it through my
usual battery of tests on all the platforms. I'll do that after patch
3 gets figured out.
Some thoughts below...
On Tue, Oct 16, 2012 at 02:06:28PM +0200, Maarten Lankhorst wrote:
> + /* ALSA is retarded, add a timeout.. */
> + ret = WaitForSingleObject(dev->sleepev, dev->sleeptime);
No arguments here :) but I'm curious what you mean. Presumably
winealsa isn't sending events when it ought to? I know Jorg has
discovered that failure, I wonder if it's worth testing and fixing
across all the drivers.
> diff --git a/dlls/dsound/primary.c b/dlls/dsound/primary.c
> index 94bdf9c..9d9fa27 100644
> --- a/dlls/dsound/primary.c
> +++ b/dlls/dsound/primary.c
> @@ -170,6 +172,7 @@ HRESULT DSOUND_ReopenDevice(DirectSoundDevice *device,
> BOOL forcewave)
> device->pwfx->wFormatTag = WAVE_FORMAT_PCM;
> device->pwfx->cbSize = 0;
> }
> +IAudioClient_SetEventHandle(device->client, device->sleepev);
>
> hres = IAudioClient_GetService(device->client, &IID_IAudioRenderClient,
> (void**)&device->render);
> @@ -203,6 +206,19 @@ HRESULT DSOUND_ReopenDevice(DirectSoundDevice *device,
> BOOL forcewave)
> WARN("GetService failed: %08x\n", hres);
> return hres;
> }
> +/* Now kick off the timer so the event fires periodically */
> +IAudioClient_Start(device->client);
> +
> +IAudioClient_GetStreamLatency(device->client, &period);
I'm a big fan of error checking (or at least reporting), so these
unchecked calls make me nervous. They'll probably never fail, but if
they do, I think they should fail loudly.
Andrew
Re: [PATCH 3/6] dsound: create a primary_pwfx separately from pwfx
Patches 1 and 2 in this series look fine. I have a series of patches similar to this one in my dsound multichannel branch. This patch seems to do too much at once. When I did this cleanup, I split it up into four patches: 1) Allocate the device format in the Device struct 2) Load the default format from the IAC, not hard-coded in dsound 3) Put SetFormat() calls into their own WFX that we don't really care about 4) Always use WAVE_FORMAT_EXTENSIBLE for the device format Much more bisect-able that way. I've attached a tarball here. It's not submission-quality yet (I haven't reviewed it lately; it might not even work), but it should show you what I had in mind for this kind of cleanup. I can bump those up in priority, but I don't think this blocks patches 4-6 in this series anyway. Andrew ds_mc.tar.gz Description: Binary data
Re: [PATCH 10/25] mciseq: Limit concurrency when starting to play.
That's a lot of patches! In the future, please try to send no more
than about 5 at a time, until each batch is committed. That's much
easier to review.
I reviewed the first 12, and will do the rest after those are
committed. Just reading the patches, I didn't notice any
show-stoppers. Overall looks good to me. A couple of thoughts below.
On Thu, Oct 04, 2012 at 01:49:13PM +0200, joerg-cyril.hoe...@t-systems.com
wrote:
>+switch (wmm->dwStatus) {
>+case MCI_MODE_PLAY:
>+case MCI_MODE_PAUSE:
>+ wmm->dwStatus = MCI_MODE_STOP;
>+}
This is weird. You're "missing" a break, and it seems like an if
statement would be more appropriate here, anyway.
Aside from that, you introduce a scary magic number, 0x78B0. I guess
that's the MIDI sound off + control message. It would be nice if it
was a proper symbol, but native's headers don't give us MIDI symbols,
and there are a lot to define. So maybe it's not worth it. Anyway,
something to think about cleaning up in the future.
Thanks for improving this code!
Andrew
Re: [PATCH] dmusic: Add master clock tests.
On Sun, Sep 23, 2012 at 08:44:59PM +0200, Christian Costa wrote:
> @@ -66,6 +68,11 @@ static void test_dmusic(void)
> return;
> }
>
> +hr = IDirectMusic_GetMasterClock(dmusic, &guid_clock, &clock);
> +ok(hr == S_OK, "IDirectMusic_GetMasterClock returned: %x\n", hr);
> +ok(clock != NULL, "No clock returned\n");
> +trace(" guidPort = %s\n", debugstr_guid(&guid_clock));
> +
Minor nit-pick: you never release the clock.
very slow updating of client area
Hello,
Please see below a function that processes WM_PAINT message. Being run
under Wine this function updates the client area (50x120 chars) about
50 times slower than under Windows XP. Could anyone please explain me
what might be wrong and suggest how to fix that?
Thank you,
Andrew Makhorin
/***
* update_panel_area - update client area of panel window
*
* This routine updates the client area of the window associated with
* the specified panel. (It is called when the panel window procedure
* receives the message WM_PAINT.) */
void update_panel_area(PANELX *px)
{ PAINTSTRUCT ps;
HFONT old_hfont;
HPEN old_hpen, hpen;
int row, col, row1, row2, col1, col2;
xassert(px != NULL);
/* if the update region is empty, do nothing */
if (!GetUpdateRect(px->window, NULL, FALSE))
goto done;
/* start updating */
BeginPaint(px->window, &ps);
old_hfont = SelectObject(ps.hdc, px->hfont);
hpen = CreatePen(PS_SOLID, 0, 0x00);
old_hpen = SelectObject(ps.hdc, hpen);
/* given the update region, determine which rows and columns of
the display area should be updated (note that the update
region reported by BeginPaint does not include right-most and
bottom-most pixels, so we subtract 1 in order not to update
an extra row and column out of the update region) */
row1 = 1 + ps.rcPaint.top / px->fy;
row2 = 1 + (ps.rcPaint.bottom - 1) / px->fy;
col1 = 1 + ps.rcPaint.left / px->fx;
col2 = 1 + (ps.rcPaint.right - 1) / px->fx;
/* updating is performed according to information in the buffer
array of the panel display area (see rpsed.h) */
for (row = row1; row <= row2; row++)
{ for (col = col1; col <= col2; col++)
{ FMTCHAR c;
int x, y;
char buf[1];
/* get formatted character at position (row,col) */
if (1 <= row && row <= px->p->nrows+1 &&
1 <= col && col <= px->p->ncols+1)
c = px->p->c[row][col];
else
c = px->p->std_char[FMT_SPACE];
/* paint this character */
x = (col - 1) * px->fx;
y = (row - 1) * px->fy;
SetTextColor(ps.hdc, palette->value[c.f]);
SetBkColor(ps.hdc, palette->value[c.b]);
SetBkMode(ps.hdc, OPAQUE);
buf[0] = c.c;
TextOut(ps.hdc, x, y, buf, 1);
/* paint the underline, if specified */
if (c.a & FMT_UNDERLINE)
{ SetBkMode(ps.hdc, TRANSPARENT);
TextOut(ps.hdc, x, y, "_", 1);
}
/* paint the vertical bar, if specified */
if (c.a & FMT_MARGIN)
{ MoveToEx(ps.hdc, x, y, NULL);
LineTo(ps.hdc, x, y + px->fy);
}
}
}
/* finish updating */
SelectObject(ps.hdc, old_hfont);
SelectObject(ps.hdc, old_hpen);
DeleteObject(hpen);
EndPaint(px->window, &ps);
done: return;
}
Re: Investigating iexplore/activex issues
On Thu, Jul 26, 2012 at 07:32:10PM +0100, Joel Holdsworth wrote: > Does anyone have any suggestions for how I can better investigate > these issues? And what is the status of ActiveX in wine - is it > likely to work at all? > I've done only a little work with ActiveX in Wine, but I think it "works" in general, but many specific cases are missing. Also remember that ActiveX programs are basically just programs, so all of the usual Wine pitfalls apply here, as well. Do you know how the ActiveX object is created? is for jscript/JavaScript objects, and it ought to "work." I don't know about vbscript support for ActiveX objects. I think most ActiveX objects will eventually be invoked as TypeLibs, in (typelib2.c is for typelib creation, which is probably not being used here). typelib.c is in working-but-hideous shape. I know typelib.c moderately well, so I might be able to help answer questions if your problem turns up in that area. You may also have to look into one of the various IDispatch implementations, like . Anyway, hope that helps guide you towards the right source files and log channels, at least. Andrew
Re: dsound: Add detection of output format and allow a maximum of 8 channels
On Sat, Jul 14, 2012 at 05:00:02PM +1000, Donny Yang wrote:
> Andrew, I've removed the two driver edits. IAudioClient_GetMixFormat()
> needs an initialised client parameter which doesn't exist in
> DirectSoundDevice_Create(), so it can only be done after the client is
> initialised, which is approximately where I put the code.
>
No, you can call GetMixFormat with an IAudioClient that hasn't been
initialized.
> Also, I'm new to this, so is this the correct way to resend a patch?
>
No, you'll have to resend the patch to wine-patches. It's probably
best to resend the entire series, and of course be sure to test your
changes before sending :)
Thanks,
Andrew
> On Fri, Jul 13, 2012 at 11:10 PM, Andrew Eikum wrote:
> > Thanks, Donny. Nice work overall. Some comments below.
> >
> > On Fri, Jul 13, 2012 at 05:57:48PM +1000, Donny Yang wrote:
> >> This patch makes dsound automatically get the output format when a
> >> output device is initialised and also allows up to 8 output channels
> >> to be used for ALSA and PulseAudio. Using ALSA with over 2 channels
> >> outputs the channels in the wrong speakers with my testing but I don't
> >> know why so I can't fix that.
> >
> > You probably already discovered that ALSA's multi-channel support is
> > primitive at best. You have to test each channel and hand-craft an
> > asoundrc to match your hardware. See some more information here:
> > <http://drona.csa.iisc.ernet.in/~uday/alsamch.shtml>
> >
> >> @@ -1492,6 +1493,24 @@ HRESULT
> >> DirectSoundDevice_Initialize(DirectSoundDevice ** ppDevice, LPCGUID lpcG
> >> } else
> >> WARN("DSOUND_PrimaryCreate failed: %08x\n", hr);
> >>
> >> +hr = IAudioClient_GetMixFormat(device->client, &pwfx);
> >> +if(FAILED(hr)){
> >> +WARN("IAudioClient_GetMixFormat failed: %08x; Falling back to
> >> default output format\n", hr);
> >> +}else{
> >> +DWORD oldPriolevel = device->priolevel;
> >> +device->priolevel = DSSCL_WRITEPRIMARY;
> >> +hr = primarybuffer_SetFormat(device, pwfx);
> >> +device->priolevel = oldPriolevel;
> >
> > This looks like a hack. I think it fits better in
> > DirectSoundDevice_Create, where the default values are copied in. Is
> > there a reason it couldn't be done there?
> >
> >> -void put_mono2stereo(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD
> >> channel, float value)
> >> +void put_mono(const IDirectSoundBufferImpl *dsb, DWORD pos, DWORD
> >> channel, float value)
> >> {
> >> -dsb->put_aux(dsb, pos, 0, value);
> >> -dsb->put_aux(dsb, pos, 1, value);
> >> +/* XXX: Is this how Windows does this? */
> >> +DWORD c;
> >> +for (c = 0; c < dsb->device->pwfx->nChannels; ++c)
> >> +dsb->put_aux(dsb, pos, c, value);
> >> }
> >>
> >
> > I think this is fine. I wonder if we should skip the LOWFREQ channel,
> > though we'd have to be careful not to affect performance too much, as
> > this runs in a really tight loop.
> >
> >> void mixieee32(float *src, float *dst, unsigned samples)
> >> diff --git a/dlls/dsound/dsound_main.c b/dlls/dsound/dsound_main.c
> >> index 072b25a..538534d 100644
> >> --- a/dlls/dsound/dsound_main.c
> >> +++ b/dlls/dsound/dsound_main.c
> >> @@ -642,7 +642,7 @@ DirectSoundCaptureEnumerateW(
> >> */
> >>
> >> typedef HRESULT (*FnCreateInstance)(REFIID riid, LPVOID *ppobj);
> >> -
> >> +
> >> typedef struct {
> >> IClassFactory IClassFactory_iface;
> >> REFCLSID rclsid;
> >> @@ -702,7 +702,7 @@ static HRESULT WINAPI DSCF_CreateInstance(
> >> *ppobj = NULL;
> >> return This->pfnCreateInstance(riid, ppobj);
> >> }
> >> -
> >> +
> >> static HRESULT WINAPI DSCF_LockServer(LPCLASSFACTORY iface, BOOL dolock)
> >> {
> >> IClassFactoryImpl *This = impl_from_IClassFactory(iface);
> >
> > The only changes to this file are whitespace changes. I'd exclude
> > these changes when you resend.
> >
> >> diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h
> >> index 1cf6daa..68f6242 100644
> >> --- a/dlls/dsound/dsound_private.h
> >> +++ b/dlls/dsound/dsound_private.h
> >> @@ -202,7 +202,7 @@ struct IDirectSoundBufferImpl
> >> };
Re: dsound: Add support for adjusting volume of more than 2 channels
On Fri, Jul 13, 2012 at 05:59:03PM +1000, Donny Yang wrote:
> - if (channels != 1 && channels != 2)
> - {
> - FIXME("There is no support for %u channels\n", channels);
> + if (dsb->device->pwfx->wFormatTag == WAVE_FORMAT_EXTENSIBLE)
> + channelMask =
> ((WAVEFORMATEXTENSIBLE*)dsb->device->pwfx)->dwChannelMask;
> + else if (channels == 1)
> + channelMask = SPEAKER_FRONT_CENTER;
> + else if (channels == 2)
> + channelMask = SPEAKER_FRONT_LEFT | SPEAKER_FRONT_RIGHT;
> + else {
> + FIXME("There is no support for %u channels without an
> extensible wave format\n", channels);
> return;
> }
Before you resend, please consider testing how Windows behaves in this
condition. Does it actually refuse to apply any volume at all?
Andrew
Re: dsound: Add detection of output format and allow a maximum of 8 channels
ioClient *iface,
> goto exit;
> }
> if(max > 8)
> -max = 2;
> +max = 8;
> if(fmt->nChannels > max){
> hr = S_FALSE;
> closest->nChannels = max;
> @@ -1649,8 +1649,8 @@ static HRESULT WINAPI
> AudioClient_GetMixFormat(IAudioClient *iface,
> goto exit;
> }
>
> -if(max_channels > 2)
> -fmt->Format.nChannels = 2;
> +if(max_channels > 8)
> +fmt->Format.nChannels = 8;
> else
> fmt->Format.nChannels = max_channels;
>
This needs to be a separate patch. Also, winealsa's multi-channel
handling needs a closer examination. My current thinking is
GetMixFormat should just return some sane, supported default (2ch,
48kHz, 16bps, or less as supported). It seems impossible to detect
what the ALSA device actually supports, so we probably shouldn't even
try like we do now.
> diff --git a/dlls/winepulse.drv/mmdevdrv.c b/dlls/winepulse.drv/mmdevdrv.c
> index b374b53..e86ed08 100644
> --- a/dlls/winepulse.drv/mmdevdrv.c
> +++ b/dlls/winepulse.drv/mmdevdrv.c
> @@ -1050,13 +1050,13 @@ static HRESULT pulse_spec_from_waveformat(ACImpl
> *This, const WAVEFORMATEX *fmt)
> This->ss.format = PA_SAMPLE_INVALID;
> switch(fmt->wFormatTag) {
> case WAVE_FORMAT_IEEE_FLOAT:
> -if (!fmt->nChannels || fmt->nChannels > 2 || fmt->wBitsPerSample !=
> 32)
> +if (!fmt->nChannels || fmt->nChannels > 8 || fmt->wBitsPerSample !=
> 32)
> break;
> This->ss.format = PA_SAMPLE_FLOAT32LE;
> pa_channel_map_init_auto(&This->map, fmt->nChannels,
> PA_CHANNEL_MAP_ALSA);
> break;
> case WAVE_FORMAT_PCM:
> -if (!fmt->nChannels || fmt->nChannels > 2)
> +if (!fmt->nChannels || fmt->nChannels > 8)
> break;
> if (fmt->wBitsPerSample == 8)
> This->ss.format = PA_SAMPLE_U8;
Wine doesn't have a PulseAudio driver yet, so this chunk won't apply.
Andrew
Re: [PATCH] winepulse.drv: Add PulseAudio driver
On Mon, Jun 18, 2012 at 08:49:55AM -0700, Chris Robinson wrote: > On Monday, June 18, 2012 9:31:04 AM Andrew Eikum wrote: > > Yeah, I experimented with ADJUST_LATENCY, as it seems to be the trick > > to getting lower latencies. Then we have to maintain our own buffer, > > which is why I switched to using the write callback, to have Pulse > > tell us when it's ready for more data from our internal buffer. > > You shouldn't need a shadow buffer with ADJUST_LATENCY. Just write it to > pulse > as the app writes it to mmdevapi. Why delay? It can handle the buffer sizes. > Are you sure? You said above: > Right, which is why you can't rely on what PulseAudio sets for the > buffer/tlength size... Is there any guarantee that Pulse /will/ give us a sufficient buffer? >From the API docs[1], it just "tries to assure" that we have the requested buffer size, which actually means nothing. And even worse, Pulse's API doesn't let us set both the buffer size and the latency[2]. Since we need to set the latency, we can't request a buffer size at all. So we have to rely on Pulse's default buffer size, and I don't think we can depend on that being sufficient in every single case and configuration. So we need the local buffer. > This also avoids the timing problems with the write callback. The most you'll > need is a temporary storage buffer for when an app writes an incomplete > period. > Yeah, the new design would only use the local buffer if not all of the data fed to mmdevapi will fit into the Pulse buffer. I remember trying this method and it failing for some reason, but maybe it'll work this time around. [1] At least, I think that's what the docs say. I still have no idea what these structure members actually do. <http://freedesktop.org/software/pulseaudio/doxygen/structpa__buffer__attr.html> [2] <http://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/Clients/LactencyControl>
Re: [PATCH] winepulse.drv: Add PulseAudio driver
On Fri, Jun 15, 2012 at 06:23:57PM -0700, Chris Robinson wrote: > The update_size is only relevant if mmdevapi updates in period-sized chunks. > If it doesn't, you can just remove it and the rounding. The tlength should be > kept updated using pa_stream_set_buffer_attr_callback, in case the server > tries to increase it more during runtime. > We chatted a little on IRC this weekend, but thanks again for the advice. Mmdevapi sends it data to the lower systems in period-sized chunks, and the reported buffer fill level decreases in period-sized chunks. But the audio clock position is more accurate than that, returning some value between periods, and taking into account latency. Unfortunately, PulseAudio (and, apparently, every Linux audio API) refuses to make any guarantees at all with regard to things like buffer sizes, period sizes, callback regularity, and latencies. So in Wine we have to build an emulation layer, involving a buffer in the driver itself, and lie about buffer drain behavior and latencies. I really don't understand why the audio APIs refuse to do this heavy lifting, instead pushing it off to applications. What is the API for if I have to do all the hard work anyway? It'd be handy if there was some page explaining how PulseAudio's data model works. Stuff like where the data is stored, when and how it's transfered, when callbacks are triggered, what the members of pa_buffer_attr actually mean (their documentation is useless), how the stream flags affect stream operation and the callbacks, when the buffer attributes might change and how applications should deal with those changes. This stuff is all unclear to me, and I think it's a big source of my frustration with the API. > When creating the stream, I set minreq to the period size, and tlength to the > requested buffer size. I also set the flags PA_STREAM_INTERPOLATE_TIMING, > PA_STREAM_AUTO_TIMING_UPDATE, and PA_STREAM_ADJUST_LATENCY. > > The most important part is to not rely on PA_STREAM_EARLY_REQUESTS or the > write callback to tell you when some space is free for writing, since as > noted > above, PulseAudio can set unreasonably large values. > Yeah, I experimented with ADJUST_LATENCY, as it seems to be the trick to getting lower latencies. Then we have to maintain our own buffer, which is why I switched to using the write callback, to have Pulse tell us when it's ready for more data from our internal buffer. But then I couldn't get the callbacks to actually trigger without supplying EARLY_REQUESTS, which invalidates our latency request, causing the high latency problem... wonderful API, isn't it? Your mail makes me think I should go back to the pulse-independent timer setup. That is, write to Pulse during the CreateTimerQueueTimer() callback. I have a strong feeling I've been down that path before, but I could give it another shot. Thanks again, Andrew
Re: [PATCH (try2)] winepulse.drv: Add PulseAudio driver
On Thu, Jun 14, 2012 at 02:46:37PM -0500, Rosanne DiMesio wrote: > A forum user reported this behavior with the Ubuntu 12.04 wine1.5 packages, > which are apparently only available with the winepulse patch. > > http://forum.winehq.org/viewtopic.php?t=15747 > Thanks. I was able to reproduce it on a Debian 6 VM, as well. Now I just need to find a better way to check if PA is running. Andrew
Re: [PATCH (try2)] winepulse.drv: Add PulseAudio driver
On Wed, Jun 13, 2012 at 09:02:33PM +0200, Alexandre Julliard wrote:
> Andrew Eikum writes:
>
> > The configure.ac changes and parts of the driver itself were written by
> > Maarten Lankhorst.
>
> It doesn't work here, it's apparently using the driver even though
> PulseAudio is not running on this box:
>
I've tried this on two operating systems, one with libpulse 2.0 and
one with libpulse 1.1, and I can't reproduce it. It always works
correctly.
Also, I should change this to load libpulse at runtime instead of
compile time, right? It works fine as-is, except it prints a
load_builtin_dll err. I'll do that for the next version of the patch.
> ../../../tools/runtest -q -P wine -M amstream.dll -T ../../.. -p
> amstream_test.exe.so amstream.c && touch amstream.ok
> err:pulse:AudioClient_Initialize PulseAudio no longer running?
> err:quartz:DSoundRender_create Cannot create Direct Sound object (88890010)
> fixme:ole:CoCreateInstance no instance created for interface
> {56a86895-0ad4-11ce-b03a-0020af0ba770} of class
> {79376820-07d0-11cf-a24d-0020afd79767}, hres is 0x88890010
> amstream.c:274: Test failed: IAMMultiMediaStream_AddMediaStream returned:
> 88890010
>
> --
> Alexandre Julliard
> julli...@winehq.org
Re: [PATCH variant 1] dsound: use a low-quality FIR for games
Thanks Alexander. Thoughts below... On Sat, May 19, 2012 at 09:09:35PM +0600, Alexander E. Patrakov wrote: > There are two ways to implement a high-performance resampler, and I > have prepared (conflicting, pick no more than one) patches for both: > > 1 (this patch): Use a shorter FIR with the existing code. This has the > advantage of higher quality (unwanted frequencies are at least > attempted to be rejected) and almost no new code. > 2 (the other patch): Write new code. E.g., linear interpolation. This > is what Windows XP does at its lowest quality setting, and it eats > less CPU than variant 1. > Do you have an opinion on which of these patches to use? The low-quality FIR has the advantage of not introducing another codepath. On the other hand, the linear resampler codepath is very simple, and even easier on the CPU. I'm leaning towards the linear resampler for its larger CPU usage benefits. > Also note that, as evicenced by the debugging patch, a Core 2 Duo > E6420 @ 2.13 GHz _can_ resample more than 32 streams simultaneously > from various weird rates to 48000 Hz. As GTA:SA reportedly creates > only 16 secondary buffers, it _should_ have more than enough CPU time > to mix them. IMHO, this makes bug #30639 look somewhat strange: on > GyB's computer, GTA:SA stutters, while Darwinia (which looks more > demanding about sound) doesn't. It may well be that in fact none of my > patches are needed, and that the real bug is that the CPU-intensive > cp_fields() function is called from a wrong thread or process. I don't > have the expertise needed to debug this. > I did some research on this. Darwinia creates up to 32 buffers, like you said. GTA:SA creates and destroys buffers as needed, and I saw it go as high as 31 in a quick test. Darwinia's buffer frequencies range in the 40-90 kHz range and resample to 22050 Hz, while GTA:SA's range around 10-20 kHz and resample to 48 kHz. So in each time step, GTA:SA requires about 1000-2000 get_current_sample() calls, but 4800 FIR convolutions per buffer. Darwinia requires 4000-9000 get_current_sample() calls, but only about 2200 convolutions per buffer. I suspect the convolutions are considerably more expensive than the get_current_sample() calls, so I would actually expect GTA:SA to be more CPU taxing. That should explain what's going on here. We could test this on Gyb's machine by setting DefaultSampleRate=22050 and hacking to return S_OK without actually changing the primary buffer's format. That should give GTA:SA similar cp_fields performance to Darwinia, and I expect it would fix the lag issue. Andrew
Re: [PATCH 1/4] dsound: Remove minlen, since we always mix full chunks anyway
On Tue, May 15, 2012 at 12:33:25PM +0600, Alexander E. Patrakov wrote:
> Andrew Eikum wrote:
> > -
> > - TRACE("Mixed at least %d from all buffers\n", minlen);
> > - return minlen;
>
> Hi. You have removed the message that could say "Mixed at least 0 from
> all buffers" (as seen, e.g., for Armored Fist 3 and sometimes even for
> foobar2000). What was the reason for that "0" (or, rather, 1024, 1024,
> 1024, 0, 2048, 1024, ... sequence) seen in some games? Was it also fixed
> by your patch?
>
That message was made redundant with the one at the beginning of
MixToPrimary, so I removed it. I don't know what causes that hiccup,
but it should still show up in log files in the first MixToPrimary
TRACE.
Andrew
Re: [PATCH] winmm: Don't call MMDevAPI during process exit
On Thu, May 10, 2012 at 08:29:25AM +0200, Alexandre Julliard wrote: > Andrew Eikum writes: > > > --- > > This fixes bug 30631, introduced by > > 31291cdc6ccc4c172ccf86f383c6a90f31a50ba1. > > > > In addition to safely exiting on process exit, we do much more > > thorough cleanup on process detach. > > There shouldn't be any reason to do cleanup on process exit (and you > can't really do anything reliably at that point anyway). > Commit 31291cdc6 introduced some process exit code. This patch removes it again. There is some other cleanup that happens on process exit, but that's been there for years. Do you want me to change the patch to remove that old code as well? Andrew
Re: [PATCH] dmusic: Variable spelling fix.
On Thu, May 10, 2012 at 07:52:58AM +0200, Christian Costa wrote:
> static HRESULT read_from_stream(IStream *stream, void *data, ULONG size)
> {
> -ULONG readed;
> +ULONG read;
> HRESULT hr;
>
> -hr = IStream_Read(stream, data, size, &readed);
> +hr = IStream_Read(stream, data, size, &read);
> if(FAILED(hr)){
I called it that to prevent shadowing read(3). It probably doesn't
matter since we almost never use that call in Wine, but that was the
rationale.
Andrew
Re: [PATCH] winealsa.drv: Init *num to 0 (Coverity)
On Mon, Apr 16, 2012 at 11:53:00AM -0500, Andrew Eikum wrote:
> On Mon, Apr 16, 2012 at 09:09:58AM +0200, Marcus Meissner wrote:
> > Also initialize a "may be uninitialized" value the compiler sees.
> >
Of course I quoted the wrong section of your mail :)
>
> Not sure what this means. *num is initialized at the beginning of
> alsa_enum_devices, which is always called from GetEndpointIDs. Seems
> like a Coverity oversight. I'd either leave it as-is, or also remove
> the *num=0 from alsa_enum_devices.
>
> Andrew
>
> > Ciao, Marcus
> > ---
> > dlls/winealsa.drv/mmdevdrv.c |3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/dlls/winealsa.drv/mmdevdrv.c b/dlls/winealsa.drv/mmdevdrv.c
> > index e3c1d15..d84bbb2 100644
> > --- a/dlls/winealsa.drv/mmdevdrv.c
> > +++ b/dlls/winealsa.drv/mmdevdrv.c
> > @@ -345,7 +345,7 @@ static WCHAR *construct_device_id(EDataFlow flow, const
> > WCHAR *chunk1, const cha
> > {
> > WCHAR *ret;
> > const WCHAR *prefix;
> > -DWORD len_wchars = 0, chunk1_len, copied = 0, prefix_len;
> > +DWORD len_wchars = 0, chunk1_len = 0, copied = 0, prefix_len;
> >
> > static const WCHAR dashW[] = {' ','-',' ',0};
> > static const size_t dashW_len = (sizeof(dashW) / sizeof(*dashW)) - 1;
> > @@ -583,6 +583,7 @@ HRESULT WINAPI AUDDRV_GetEndpointIDs(EDataFlow flow,
> > WCHAR ***ids, GUID **guids,
> >
> > TRACE("%d %p %p %p %p\n", flow, ids, guids, num, def_index);
> >
> > +*num = 0;
> > *ids = NULL;
> > *guids = NULL;
> >
> > --
> > 1.7.3.4
> >
> >
> >
>
>
Re: [PATCH] winealsa.drv: Init *num to 0 (Coverity)
On Mon, Apr 16, 2012 at 09:09:58AM +0200, Marcus Meissner wrote:
> Also initialize a "may be uninitialized" value the compiler sees.
>
Not sure what this means. *num is initialized at the beginning of
alsa_enum_devices, which is always called from GetEndpointIDs. Seems
like a Coverity oversight. I'd either leave it as-is, or also remove
the *num=0 from alsa_enum_devices.
Andrew
> Ciao, Marcus
> ---
> dlls/winealsa.drv/mmdevdrv.c |3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/dlls/winealsa.drv/mmdevdrv.c b/dlls/winealsa.drv/mmdevdrv.c
> index e3c1d15..d84bbb2 100644
> --- a/dlls/winealsa.drv/mmdevdrv.c
> +++ b/dlls/winealsa.drv/mmdevdrv.c
> @@ -345,7 +345,7 @@ static WCHAR *construct_device_id(EDataFlow flow, const
> WCHAR *chunk1, const cha
> {
> WCHAR *ret;
> const WCHAR *prefix;
> -DWORD len_wchars = 0, chunk1_len, copied = 0, prefix_len;
> +DWORD len_wchars = 0, chunk1_len = 0, copied = 0, prefix_len;
>
> static const WCHAR dashW[] = {' ','-',' ',0};
> static const size_t dashW_len = (sizeof(dashW) / sizeof(*dashW)) - 1;
> @@ -583,6 +583,7 @@ HRESULT WINAPI AUDDRV_GetEndpointIDs(EDataFlow flow,
> WCHAR ***ids, GUID **guids,
>
> TRACE("%d %p %p %p %p\n", flow, ids, guids, num, def_index);
>
> +*num = 0;
> *ids = NULL;
> *guids = NULL;
>
> --
> 1.7.3.4
>
>
>
Re: WASAPI ISimpleAudioVolume::SetMasterVolume appears to not work
Hi Adam, On Sun, Apr 15, 2012 at 12:06:14AM +0100, adam smith wrote: > I've been trying to get WASAPI audio output working for our app as > Directsound output was very glitchy post the new audio stack. > Sorry to hear that. We do have a large sequence of patches just about ready to go into Wine which will improve directsound greatly, but they're not in quite yet. > All seems to work great with WASAPI however I need to able to set the > volume level independently for individual audio sessions / streams for > crossfading however I'm having problems controlling the volume levels of a > shared mode audio session / stream. specifically, > > ISimpleAudioVolume::SetMasterVolume ... returns success but does not > change the volume level though interestingly > Yes, this is a limitation of the ALSA and OSS drivers. You can see that actually doesn't do anything. This is because ALSA and OSS don't support per-stream volume control. For example, if we set the volume on an ALSA device, it will actually affect the volume of _every_ stream that uses that ALSA device, which isn't what we want. > HRESULT hr; > hr=audioClient_->GetService(IID_ISimpleAudioVolume,(void**)&simpleAudioVolume_); > if (SUCCESS(hr)) > hr=simpleAudioVolume_->SetMute(TRUE,NULL); > > does work and mute the volume. > Yes, we have some logic to allow muting to work, by just setting the samples sent to ALSA to silence. Again, this is because muting the ALSA device would result in muting _every_ stream using that device. > Does anyone have any experience of these interfaces or know equivalent > interfaces to use instead > Sorry, Linux's audio systems just don't have the capabilities to let Wine do this. We would need to build a software mixer into Wine to do volume control, and we don't have that (yet). Andrew
Re: winealsa: Fix an uninitialized variable compiler warning.
Thanks. GCC 4.7 must be smart enough to detect the same if-conditions,
and doesn't issue a warning for me. Kind of impressive, actually.
Andrew
On Fri, Apr 06, 2012 at 02:49:38AM +0200, Józef Kucia wrote:
> ---
> dlls/winealsa.drv/mmdevdrv.c |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/dlls/winealsa.drv/mmdevdrv.c b/dlls/winealsa.drv/mmdevdrv.c
> index c99186b..1438f5b 100644
> --- a/dlls/winealsa.drv/mmdevdrv.c
> +++ b/dlls/winealsa.drv/mmdevdrv.c
> @@ -343,7 +343,7 @@ static WCHAR *construct_device_id(EDataFlow flow, const
> WCHAR *chunk1, const cha
> {
> WCHAR *ret;
> const WCHAR *prefix;
> -DWORD len_wchars = 0, chunk1_len, copied = 0, prefix_len;
> +DWORD len_wchars = 0, chunk1_len = 0, copied = 0, prefix_len;
>
> static const WCHAR dashW[] = {' ','-',' ',0};
> static const size_t dashW_len = (sizeof(dashW) / sizeof(*dashW)) - 1;
> --
> 1.7.8.5
>
>
>
Re: [PATCH 3/3] dmsynth: Put port caps that match native ones.
On Fri, Mar 30, 2012 at 12:33:51PM +0200, joerg-cyril.hoe...@t-systems.com wrote: > Andrew Eikum asked Christian Costa: > >Just to be clear, which "native" do you mean? Does this match Windows > >7's dmusic behavior, or something else? I've been trying to target > >Windows 7's behavior in the new audio design. > > A w7 target makes a lot of sense for mmdevapi/WASAPI. > > However, isn't DMusic a thing of the past? In that case, I argue that DSound > and DMusic > should target the "best in class" native OS of the time where apps were > written > using these APIs, because the app writers "optimized" their apps to the > behaviour > they could observe. Wouldn't "best in class" mean XP for DSound/DMusic? > > For similar reasons, the MCI should mimic w9x(/wxp) behaviour, because that's > what > was available when apps using the MCI were developed. > > I prefer to hear "thank you guys, app X works like a charm in Wine like it > worked on > my rusty w98 and later xp box, whereas it refuses to run on w7" rather than > "you guys do a great job of making my app X worked as badly in Wine as in w7 > :-( > whereas it used to work fine on my w98/xp machine". > I think about the goal of "targeting Windows 7" as more about revealing how Microsoft chose to implement these APIs on top of MMDevAPI, than about replicating Windows 7's behavior exactly. Targeting Windows 7 is the default choice when implementing these APIs in Wine, and when we find applications that expect older behavior, we can deviate from Windows 7's behavior to fix them specifically. Andrew
Re: [PATCH 2/5] dmusic: Enumerate ports for midi out and midi in devices in EnumPort.
On Thu, Mar 29, 2012 at 04:18:20PM +0200, Christian Costa wrote:
> It's the original code. I decided to keep it in this patch and add error
> checking in the patch that comes just after.
> I can merge them but I prefer to do things incrementally as much as
> possible.
Ah, sure enough. I missed that patch. Though in [3/5], you're missing
assigning hr for _GetPortCaps(). I also think it would be nice to WARN
with the result code in the failure case (or TRACE if a failure is
expected in some cases). You know, the usual
"if(FAILED(hr)){WARN(..., hr); return hr;}" error checking thing.
Re: [PATCH 3/3] dmsynth: Put port caps that match native ones.
Just to be clear, which "native" do you mean? Does this match Windows 7's dmusic behavior, or something else? I've been trying to target Windows 7's behavior in the new audio design. Also... On Thu, Mar 29, 2012 at 08:53:52AM +0200, Christian Costa wrote: > - obj->pCaps.dwEffectFlags = DMUS_EFFECT_REVERB | DMUS_EFFECT_CHORUS | > DMUS_EFFECT_DELAY; > + obj->pCaps.dwEffectFlags = DMUS_EFFECT_REVERB; /* DMUS_EFFECT_CHORUS | > DMUS_EFFECT_DELAY */ I'd just remove the unused flags. No need for crufty comments.
Re: [PATCH 2/5] dmusic: Enumerate ports for midi out and midi in devices in EnumPort.
On Thu, Mar 29, 2012 at 08:52:45AM +0200, Christian Costa wrote: > +midiOutGetDevCapsW(index - 1, &caps, sizeof(caps)); > ... > +CoCreateInstance(&CLSID_DirectMusicSynth, NULL, > CLSCTX_INPROC_SERVER, &IID_IDirectMusicSynth8, (void**)&synth); > +IDirectMusicSynth8_GetPortCaps(synth, port_caps); Maybe I'm pickier than most people, but I like error checking on these sorts of calls. They make it easier to find what is happening when something goes terribly and unexpectedly wrong.
Re: [PATCH 5/5] dmusic: Implement partially SetDirectSound.
It would be nice to see some tests for this. For example, how does
this IDirectSound reference relate to the one created by
IDirectMusicPerformance::InitAudio()?
On Thu, Mar 29, 2012 at 08:53:10AM +0200, Christian Costa wrote:
>
> ---
> dlls/dmusic/dmusic.c | 27 +--
> dlls/dmusic/dmusic_private.h |2 ++
> 2 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/dmusic/dmusic.c b/dlls/dmusic/dmusic.c
> index 8f2edea..60d6b8a 100644
> --- a/dlls/dmusic/dmusic.c
> +++ b/dlls/dmusic/dmusic.c
> @@ -52,19 +52,22 @@ static ULONG WINAPI IDirectMusic8Impl_AddRef
> (LPDIRECTMUSIC8 iface) {
> return refCount;
> }
>
> -static ULONG WINAPI IDirectMusic8Impl_Release (LPDIRECTMUSIC8 iface) {
> +static ULONG WINAPI IDirectMusic8Impl_Release(LPDIRECTMUSIC8 iface)
> +{
> IDirectMusic8Impl *This = (IDirectMusic8Impl *)iface;
> ULONG refCount = InterlockedDecrement(&This->ref);
>
> TRACE("(%p)->(ref before=%u)\n", This, refCount + 1);
>
> if (!refCount) {
> + if (This->dsound)
> + IDirectSound_Release(This->dsound);
> HeapFree(GetProcessHeap(), 0, This->ppPorts);
> HeapFree(GetProcessHeap(), 0, This);
> }
>
> DMUSIC_UnlockModule();
> -
> +
> return refCount;
> }
>
> @@ -257,10 +260,22 @@ static HRESULT WINAPI IDirectMusic8Impl_GetDefaultPort
> (LPDIRECTMUSIC8 iface, LP
> return S_OK;
> }
>
> -static HRESULT WINAPI IDirectMusic8Impl_SetDirectSound (LPDIRECTMUSIC8
> iface, LPDIRECTSOUND pDirectSound, HWND hWnd) {
> - IDirectMusic8Impl *This = (IDirectMusic8Impl *)iface;
> - FIXME("(%p, %p, %p): stub\n", This, pDirectSound, hWnd);
> - return S_OK;
> +static HRESULT WINAPI IDirectMusic8Impl_SetDirectSound(LPDIRECTMUSIC8 iface,
> LPDIRECTSOUND dsound, HWND wnd)
> +{
> +IDirectMusic8Impl *This = (IDirectMusic8Impl *)iface;
> +
> +FIXME("(%p, %p, %p): partial implementation\n", This, dsound, wnd);
> +
> +if (dsound)
> +{
> +This->dsound = dsound;
> +IDirectSound_AddRef(dsound);
> +}
> +
> +/* If no direct sound object is given, native does not create one yet */
> +This->dsound_inited = TRUE;
> +
> +return S_OK;
> }
>
> static HRESULT WINAPI IDirectMusic8Impl_SetExternalMasterClock
> (LPDIRECTMUSIC8 iface, IReferenceClock* pClock) {
> diff --git a/dlls/dmusic/dmusic_private.h b/dlls/dmusic/dmusic_private.h
> index c05adf0..b0655d6 100644
> --- a/dlls/dmusic/dmusic_private.h
> +++ b/dlls/dmusic/dmusic_private.h
> @@ -96,6 +96,8 @@ struct IDirectMusic8Impl {
>IReferenceClockImpl* pMasterClock;
>IDirectMusicPort** ppPorts;
>int nrofports;
> + BOOL dsound_inited;
> + IDirectSound* dsound;
> };
>
>
> /*
>
>
>
RFC: Duplicate in/out device naming
I'm working on Bug 14559 and have run into an interesting UI problem. In WinMM, there are three "classes" of devices: waveIn, waveOut, and mixer. Windows 7's WinMM implementation provides one mixer for each waveIn and waveOut device. The "WCHAR caps.szPname[32]" field for the matching mixer and waveIn/Out device are the same. Some applications, in this case Rosetta Stone 3, use the matching szPname fields to determine which mixer to associate with which device. The problem turns up when we have duplicate input and output device names. Consider: waveOut: 0: default 1: HDA Intel Analog 2: HDA Intel Digital waveIn: 0: default 1: HDA Intel Analog mixer: 0: default 1: HDA Intel Analog 2: HDA Intel Digital 3: default 4: HDA Intel Analog Rosetta Stone chooses waveIn[0] and starts iterating over the mixers' szPname fields, finding that mixer[0] matches. But, mixer[0] is actually associated with the waveOut default, not the waveIn default. Bad things happen. Windows 7 solves this problem implicitly by providing unique device names at the MMDevAPI level. For instance, the input device is Microphone and the output devices are Stereo and Headphones. Hacking the registry to force duplicate MMDevAPI device names actually does result in duplicate mixer names. But Wine's backends don't provide this kind of information, so we simply provide the ALSA device name as the device's name. As an attempt to solve this in Wine, I added a prefix to each MMDevice indicating its flow direction. The unique identifier must be a prefix, as WinMM's szPname field truncates at 31 characters. This gives WinMM device names like: waveOut: 0: Out: default 1: Out: HDA Intel Analog 2: Out: HDA Intel Digital This solves the Rosetta Stone issue, as you can see in the bug. But when testing this in other applications, it quickly becomes silly. Audacity 1.3 adds its own "Out: " prefix to WinMM device names, creating "Out: Out: default" in the UI. Dynamically adding prefixes only if a duplicate device is present causes issues with duplicates appearing/disappearing. Trying to remember if a duplicate has ever been present is quite "ugly" in code, requiring storing that kind of information in the registry. So I don't know what to do. Have ugly, redundant device names? Leave Rosetta Stone broken, as it is arguably an application bug? Try to guess at the device type, and insert names like "Speakers" and "Microphone" into the MMDevAPI device name, even if that might be wrong? Thoughts? Andrew
Re: Instructions for using JACK with with Wine's ALSA output on wiki: reverted twice
On Mon, Mar 12, 2012 at 07:18:39AM -0600, Vitaliy Margolen wrote: > On 03/12/2012 06:11 AM, Andrew Eikum wrote: > >On Sun, Mar 11, 2012 at 10:15:40AM -0600, Vitaliy Margolen wrote: > >>That information simply does not belong on Wine wiki. It talks about > >>making Jack work as Alsa sound device. This has nothing to do with > >>Wine in particular. Especially that he posted the link to an > >>external wiki which has most of this information. > >> > >>Let me make it a bit more clear, Wine is huge and it uses lots and > >>lots of system libraries, packages, subsystems, features, and so on. > >>Unless some information directly related to Wine it should not be > >>posted on the Wine wiki. Or we'll end up with a huge collection of > >>generic Linux/OS X/BSD/ARM/etc articles that will rot over time. > >> > > > >Next time you're going to delete an entire half of a Wiki page that > >someone clearly cares about and maintains (see the edit history), > >please contact that someone first so we can discuss what should be > >done about the information that doesn't belong there. > Sorry, but I'm not going to do that. If you think that some, not > Wine related, information belongs on Wine wiki, you better bring > that question here. We already shot down a lot of attempts to host > all sort of junk, including win API documentation. Which, if > anything, more relates to Wine then how to make one sound device > work like another sound device. > So host the content elsewhere and link to it. Revert wars on the Wiki are not going to accomplish anything or make anybody happy. If you can't or aren't willing to do that, then email someone who cares about the page and ask them to clean it up. There are more options here than "delete the content without comment." See also: <http://en.wikipedia.org/wiki/Wikipedia:Assume_good_faith> We used to have JACK support, and we removed it. It makes sense to have some sort of fallback for JACK users, even if it's not supported. A very knowledgeable user found a setup that works well for him and offered that information to us. Rather than act like a jerk and tell him to go elsewhere with his information, I chose to take him up on his offer. If it doesn't belong on the Wiki in exactly that form, then let's work to find a better way to give that information to users. > >I agree that the Wiki isn't the best place for that code, and I was > >planning on getting around to fixing it. In the meantime, I had linked > >to that code in Bug 29294 and now it's not there for users to find. > >There is no emergency here that could not have been better resolved > >by a discussion over email. > What prevent you from attaching that code to the bug that needs it? > And discussion with whom? That person has no wiki personal page, nor > was he using a known nik associated with an e-mail. > If you browse the edit history for that page, you'll see that I edit it pretty frequently. Maybe I have an opinion? No lives are going to be lost if some inappropriate data is left on the Wiki for a weekend. > >The fact that you've been specifically called out in two threads in > >one weekend might be a hint that it is _your_ behavior that is > >inappropriate. > I've been called out in more then two threads, that doesn't bother > me. That just an indication that no one else wants to do this work. > Or that no one wants this work done. Andrew
Re: Instructions for using JACK with with Wine's ALSA output on wiki: reverted twice
Vitaliy, On Sun, Mar 11, 2012 at 10:15:40AM -0600, Vitaliy Margolen wrote: > That information simply does not belong on Wine wiki. It talks about > making Jack work as Alsa sound device. This has nothing to do with > Wine in particular. Especially that he posted the link to an > external wiki which has most of this information. > > Let me make it a bit more clear, Wine is huge and it uses lots and > lots of system libraries, packages, subsystems, features, and so on. > Unless some information directly related to Wine it should not be > posted on the Wine wiki. Or we'll end up with a huge collection of > generic Linux/OS X/BSD/ARM/etc articles that will rot over time. > Next time you're going to delete an entire half of a Wiki page that someone clearly cares about and maintains (see the edit history), please contact that someone first so we can discuss what should be done about the information that doesn't belong there. I agree that the Wiki isn't the best place for that code, and I was planning on getting around to fixing it. In the meantime, I had linked to that code in Bug 29294 and now it's not there for users to find. There is no emergency here that could not have been better resolved by a discussion over email. The fact that you've been specifically called out in two threads in one weekend might be a hint that it is _your_ behavior that is inappropriate. Andrew
Re: Major mmdevapi and winmm audio bugs
On Tue, Feb 28, 2012 at 08:24:37AM -0600, Andrew Eikum wrote:
> I'm investigating native TimerQueue's operation now. If it turns out
> that TimerQueue isn't sufficient, we'll probably just switch over to
> using poll() like winmm's timer stuff does.
>
And, not too surprisingly, TimerQueue isn't sufficient. I've attached
my tests here. The tests ask to execute the callback every 12 ms. On
my Windows 7 VM, I found that it executed with intervals like:
20, 10, 10, 10, 10, 20, 10, 10, 10, 10, ...
With an 11 occasionally interspersed.
So, forget that.
Winmm's timer functions use poll() with a timeout value, subtracting
the time elapsed curing the callback. This works quite well in dsound.
The Win32 API also provides the SetTimer API. But that depends on a
message queue, which is a hassle I don't know if we want to bother
with in the drivers (plus, it may be no more reliable than the
TimerQueues).
Since we're already using poll() in winmm and it seems to work well,
that would be my suggestion.
Any thoughts?
Andrew
>From 28fd9fbbff54be8589736d598092d9e0ff666343 Mon Sep 17 00:00:00 2001
From: Andrew Eikum
Date: Tue, 28 Feb 2012 09:25:40 -0600
Subject: INCOMPLETE: kernel32/tests: Test timer interval consistency
To: wine-patches
Reply-To: wine-devel ,Andrew Eikum
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="1.7.9.2"
This is a multi-part message in MIME format.
--1.7.9.2
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
---
dlls/kernel32/tests/sync.c | 45
1 file changed, 45 insertions(+)
--1.7.9.2
Content-Type: text/x-patch; name="0012-INCOMPLETE-kernel32-tests-Test-timer-interval-consis.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: inline; filename="0012-INCOMPLETE-kernel32-tests-Test-timer-interval-consis.patch"
diff --git a/dlls/kernel32/tests/sync.c b/dlls/kernel32/tests/sync.c
index 07bc2ca..8532e49 100644
--- a/dlls/kernel32/tests/sync.c
+++ b/dlls/kernel32/tests/sync.c
@@ -1064,6 +1064,50 @@ static void test_WaitForMultipleObjects(void)
if (maxevents[i]) CloseHandle(maxevents[i]);
}
+#define TICK_INTERVAL 12
+
+static void CALLBACK timing_cb(PVOID p, BOOLEAN timedOut)
+{
+DWORD *tick = p, cur;
+
+cur = GetTickCount();
+ok(0 && cur - *tick <= TICK_INTERVAL, "timing_cb was invoked late! %u - %u = %u > %u\n", cur, *tick, cur - *tick, TICK_INTERVAL);
+
+Sleep(4);
+
+*tick += TICK_INTERVAL;
+}
+
+static void test_timer_queue_timing(void)
+{
+HANDLE queue, timer;
+DWORD tick;
+BOOL br;
+
+if (!pChangeTimerQueueTimer || !pCreateTimerQueue || !pCreateTimerQueueTimer
+|| !pDeleteTimerQueueEx || !pDeleteTimerQueueTimer)
+{
+win_skip("TimerQueue API not present\n");
+return;
+}
+
+queue = pCreateTimerQueue();
+ok(queue != NULL, "CreateTimerQueue failed\n");
+
+tick = GetTickCount();
+br = pCreateTimerQueueTimer(&timer, NULL, &timing_cb, &tick, 0, TICK_INTERVAL,
+WT_EXECUTEINTIMERTHREAD);
+ok(br == TRUE, "CreateTimerQueueTimer failed\n");
+
+Sleep(2000);
+
+br = pDeleteTimerQueueTimer(queue, timer, INVALID_HANDLE_VALUE);
+ok(br == TRUE, "DeleteTimerQueueTimer failed\n");
+
+br = pDeleteTimerQueueEx(queue, INVALID_HANDLE_VALUE);
+ok(br == TRUE, "DeleteTimerQueueEx failed\n");
+}
+
START_TEST(sync)
{
HMODULE hdll = GetModuleHandle("kernel32");
@@ -1083,6 +1127,7 @@ START_TEST(sync)
test_waitable_timer();
test_iocp_callback();
test_timer_queue();
+test_timer_queue_timing();
test_WaitForSingleObject();
test_WaitForMultipleObjects();
}
--1.7.9.2--
Re: Major mmdevapi and winmm audio bugs
On Mon, Feb 27, 2012 at 11:41:56PM +0100, Maarten Lankhorst wrote: > > W.r.t. to 1.4.0, I believe the following path should be taken: > > - Write CreateTimerQueue tests to verify whether native behaves like: > > a) sleep(Xms) /* what Wine does now */ or > > b) sleep(Xms - elapsed) /* what winmm:timeSetEvent does */ > > - If b), fix CreateTimerQueue. > > - If a), CreateTimerQueue is unusable for the purposes of Wine's mmdevapi > > and something else must be sought... > Even if so, it is unneeded for the pulse driver, I'm using the auto timing > update feature and the callback for when rendering is not running. > When the buffer is not underflowed, I'm using the write callback to > signal more data is available. > > Timerqueue would be a lie, the driver should schedule when data can > actually be written and not based purely on ticks. Pulseaudio allows > this and I'm using this feature in my driver. It passes all the render > tests now except the volume related ones (they're a reminder, > and a nop on other drivers anyhow), and I'm working to complete > capture in a similar fashion as well. > > I believe the event should be triggered when there is actual data > available, at least that's how I understood the event is supposed > to work. Doing it based on a timer that isn't tied to the state in > any way feels wrong. Having 2 threads for pulse feels wrong too, > especially since it can be done with just using the one pulse > provides, because you can set callbacks for when more data > can be read or written. > Unfortunately, the only backend that actually produces that kind of information is Pulse. So we need a solution for the other drivers anyway. At that point, we'll have to decide whether we want to use the same mechanism for Pulse, or rely on the callbacks. There's an argument for having each driver behave as similar as possible, even if it's arguably less "correct." I've also been burned enough times by relying on the audio backend to work correctly that I hesitate to give the backends more responsibility than absolutely necessary. I'm investigating native TimerQueue's operation now. If it turns out that TimerQueue isn't sufficient, we'll probably just switch over to using poll() like winmm's timer stuff does. Andrew
Re: dsound: Small fixes to IDirectSoundCaptureBuffer
This patch looks sane and fixes the tests on my machine, but causes no
changes to dsound capture behavior in Audacity. I don't have much of
an opinion on if it should go in for 1.4.
On Tue, Feb 21, 2012 at 05:33:35PM +0100, Maarten Lankhorst wrote:
> ---
> I suspect a lot more AUDCLNT_E_* calls will have to be mapped
> correctly, but at least this is enough to pass the dsound:capture
> tests again on wine.
>
> dlls/dsound/capture.c |8 +---
> dlls/dsound/tests/capture.c | 24 +++-
> 2 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/dlls/dsound/capture.c b/dlls/dsound/capture.c
> index 90cddb7..7ceddfc 100644
> --- a/dlls/dsound/capture.c
> +++ b/dlls/dsound/capture.c
> @@ -456,13 +456,14 @@ static HRESULT WINAPI
> IDirectSoundCaptureBufferImpl_Start(IDirectSoundCaptureBuf
>
> EnterCriticalSection(&(This->device->lock));
>
> -This->flags = dwFlags;
> -TRACE("old
> This->device->state=%s\n",captureStateString[This->device->state]);
> if (This->device->state == STATE_STOPPED)
> This->device->state = STATE_STARTING;
> else if (This->device->state == STATE_STOPPING)
> This->device->state = STATE_CAPTURING;
> +else
> +goto out;
> TRACE("new
> This->device->state=%s\n",captureStateString[This->device->state]);
> +This->flags = dwFlags;
>
> if (This->device->buffer)
> FillMemory(This->device->buffer, This->device->buflen,
> (This->device->pwfx->wBitsPerSample == 8) ? 128 : 0);
> @@ -474,6 +475,7 @@ static HRESULT WINAPI
> IDirectSoundCaptureBufferImpl_Start(IDirectSoundCaptureBuf
> return hres;
> }
>
> +out:
> LeaveCriticalSection(&This->device->lock);
>
> TRACE("returning DS_OK\n");
> @@ -705,7 +707,7 @@ static HRESULT IDirectSoundCaptureBufferImpl_Create(
> HeapFree(GetProcessHeap(), 0, This->pdscbd);
> This->device->capture_buffer = 0;
> HeapFree( GetProcessHeap(), 0, This );
> -return err;
> +return err == AUDCLNT_E_UNSUPPORTED_FORMAT ? DSERR_BADFORMAT :
> err;
> }
>
> err = IAudioClient_GetService(device->client,
> &IID_IAudioCaptureClient,
> diff --git a/dlls/dsound/tests/capture.c b/dlls/dsound/tests/capture.c
> index 96fb417..2040f3f 100644
> --- a/dlls/dsound/tests/capture.c
> +++ b/dlls/dsound/tests/capture.c
> @@ -393,15 +393,18 @@ static void test_capture_buffer(LPDIRECTSOUNDCAPTURE
> dsco,
> ok(ref==0,"IDirectSoundNotify_Release(): has %d references, should have "
> "0\n",ref);
>
> -if (record) {
> - rc=IDirectSoundCaptureBuffer_Start(dscbo,DSCBSTART_LOOPING);
> - ok(rc==DS_OK,"IDirectSoundCaptureBuffer_Start() failed: %08x\n", rc);
> +rc=IDirectSoundCaptureBuffer_Start(dscbo,DSCBSTART_LOOPING);
> +ok(rc==DS_OK,"IDirectSoundCaptureBuffer_Start() failed: %08x\n", rc);
> +
> +rc=IDirectSoundCaptureBuffer_Start(dscbo,0);
> +ok(rc==DS_OK,"IDirectSoundCaptureBuffer_Start() failed: %08x\n", rc);
>
> - rc=IDirectSoundCaptureBuffer_GetStatus(dscbo,&status);
> - ok(rc==DS_OK,"IDirectSoundCaptureBuffer_GetStatus() failed: %08x\n",
> rc);
> - ok(status==(DSCBSTATUS_CAPTURING|DSCBSTATUS_LOOPING),
> - "GetStatus: bad status: %x\n",status);
> +rc=IDirectSoundCaptureBuffer_GetStatus(dscbo,&status);
> +ok(rc==DS_OK,"IDirectSoundCaptureBuffer_GetStatus() failed: %08x\n", rc);
> +ok(status==(DSCBSTATUS_CAPTURING|DSCBSTATUS_LOOPING),
> + "GetStatus: bad status: %x\n",status);
>
> +if (record) {
> /* wait for the notifications */
> for (i = 0; i < (NOTIFICATIONS * 2); i++) {
> rc=WaitForMultipleObjects(NOTIFICATIONS,state.event,FALSE,3000);
> @@ -416,9 +419,12 @@ static void test_capture_buffer(LPDIRECTSOUNDCAPTURE
> dsco,
> break;
> }
>
> - rc=IDirectSoundCaptureBuffer_Stop(dscbo);
> - ok(rc==DS_OK,"IDirectSoundCaptureBuffer_Stop() failed: %08x\n", rc);
> }
> +rc=IDirectSoundCaptureBuffer_Stop(dscbo);
> +ok(rc==DS_OK,"IDirectSoundCaptureBuffer_Stop() failed: %08x\n", rc);
> +
> +rc=IDirectSoundCaptureBuffer_Stop(dscbo);
> +ok(rc==DS_OK,"IDirectSoundCaptureBuffer_Stop() failed: %08x\n", rc);
> }
>
> static BOOL WINAPI dscenum_callback(LPGUID lpGuid, LPCSTR lpcstrDescription,
> --
> 1.7.7.6
>
>
>
>
>
Re: winmm: Allow new sounds to interrupt previous PlaySound invocations.
On Thu, Feb 16, 2012 at 10:10:09AM +0100, joerg-cyril.hoe...@t-systems.com wrote: > Hi, > > this fixes bug #28413. > Thanks Jörg, looks sane to me and I can confirm it fixes the problem. Andrew
Re: winealsa.drv patch to enum all software devices from .asoundrc
On Mon, Feb 13, 2012 at 12:08:57PM -0800, Chris Robinson wrote:
> For now, I think snd_card_next, with registry options for custom prefixes and
> an additional set of devices, is the best way to go. There's simply too much
> junk with snd_ctl_name_hint.
>
I agree with this analysis. Here's a patch. It uses pipes to separate
device names, since I know commas are meaningful in ALSA. Perhaps
that's not enough, and we need to do quoting?
In any case, this patch ought to be enough to satisfy users for 1.4.
Any thoughts, anyone?
Andrew
>From 9fd014ff6535493cc42e8554af9251a8729f6c73 Mon Sep 17 00:00:00 2001
From: Andrew Eikum
Date: Mon, 13 Feb 2012 09:47:03 -0600
Subject: winealsa.drv: Optionally load extra ALSA device names from the
registry
To: wine-patches
Reply-To: wine-devel ,Andrew Eikum
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="1.7.9"
This is a multi-part message in MIME format.
--1.7.9
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
---
dlls/winealsa.drv/Makefile.in |2 +-
dlls/winealsa.drv/mmdevdrv.c | 56 +++--
2 files changed, 49 insertions(+), 9 deletions(-)
--1.7.9
Content-Type: text/x-patch; name="0002-winealsa.drv-Optionally-load-extra-ALSA-device-names.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: inline; filename="0002-winealsa.drv-Optionally-load-extra-ALSA-device-names.patch"
diff --git a/dlls/winealsa.drv/Makefile.in b/dlls/winealsa.drv/Makefile.in
index 336da8b..7086ce6 100644
--- a/dlls/winealsa.drv/Makefile.in
+++ b/dlls/winealsa.drv/Makefile.in
@@ -1,5 +1,5 @@
MODULE= winealsa.drv
-IMPORTS = uuid ole32
+IMPORTS = uuid ole32 advapi32
DELAYIMPORTS = winmm
EXTRALIBS = @ALSALIBS@
diff --git a/dlls/winealsa.drv/mmdevdrv.c b/dlls/winealsa.drv/mmdevdrv.c
index 7621423..ff3cbbc 100644
--- a/dlls/winealsa.drv/mmdevdrv.c
+++ b/dlls/winealsa.drv/mmdevdrv.c
@@ -147,9 +147,6 @@ static CRITICAL_SECTION_DEBUG g_sessions_lock_debug =
static CRITICAL_SECTION g_sessions_lock = { &g_sessions_lock_debug, -1, 0, 0, 0, 0 };
static struct list g_sessions = LIST_INIT(g_sessions);
-static const WCHAR defaultW[] = {'d','e','f','a','u','l','t',0};
-static const char defname[] = "default";
-
static const IAudioClientVtbl AudioClient_Vtbl;
static const IAudioRenderClientVtbl AudioRenderClient_Vtbl;
static const IAudioCaptureClientVtbl AudioCaptureClient_Vtbl;
@@ -343,30 +340,73 @@ static HRESULT alsa_get_card_devices(snd_pcm_stream_t stream, WCHAR **ids, char
static HRESULT alsa_enum_devices(EDataFlow flow, WCHAR **ids, char **keys,
UINT *num)
{
+static const WCHAR drv_keyW[] = {'S','o','f','t','w','a','r','e','\\',
+'W','i','n','e','\\','D','r','i','v','e','r','s','\\',
+'w','i','n','e','a','l','s','a','.','d','r','v',0};
+static const char ALSAOutputDevices[] = "ALSAOutputDevices";
+static const char ALSAInputDevices[] = "ALSAInputDevices";
+static const WCHAR defaultW[] = {'d','e','f','a','u','l','t',0};
+static const char ALSA_Default[] = "default";
+
snd_pcm_stream_t stream = (flow == eRender ? SND_PCM_STREAM_PLAYBACK :
SND_PCM_STREAM_CAPTURE);
+DWORD len;
int err, card;
+HKEY key;
+char reg_devices[256];
-card = -1;
*num = 0;
-if(alsa_try_open(defname, stream)){
+if(alsa_try_open(ALSA_Default, stream)){
if(ids && keys){
*ids = HeapAlloc(GetProcessHeap(), 0, sizeof(defaultW));
memcpy(*ids, defaultW, sizeof(defaultW));
-*keys = HeapAlloc(GetProcessHeap(), 0, sizeof(defname));
-memcpy(*keys, defname, sizeof(defname));
+
+*keys = HeapAlloc(GetProcessHeap(), 0, sizeof(ALSA_Default));
+memcpy(*keys, ALSA_Default, sizeof(ALSA_Default));
}
++*num;
}
+if(RegOpenKeyW(HKEY_CURRENT_USER, drv_keyW, &key) == ERROR_SUCCESS){
+DWORD size = sizeof(reg_devices);
+const char *value_name = (flow == eRender) ? ALSAOutputDevices : ALSAInputDevices;
+
+if(RegQueryValueExA(key, value_name, 0, NULL,
+(BYTE*)reg_devices, &size) == ERROR_SUCCESS){
+char *next, *p;
+
+TRACE("Loaded ALSA device list from registry: %s\n", reg_devices);
+
+for(next = p = reg_devices
Re: winealsa.drv patch to enum all software devices from .asoundrc
On Mon, Feb 13, 2012 at 02:12:02PM +0100, joerg-cyril.hoe...@t-systems.com
wrote:
> Bottom line IMHO:
> 1. Wine-1.4 MUST have a means to select something
>other than "default" without recompiling.
> 2. Via registry or winecfg, I don't mind.
>
> A stupid registry entry would be fine with me.
Here's a stupid registry entry patch. The paths are:
HKCU\Software\Wine\Drivers\winealsa.drv\ALSAOutputDevice
HKCU\Software\Wine\Drivers\winealsa.drv\ALSAInputDevice
If we can't reach a sensible consensus about device enumeration
sometime this week, I'll submit this to fix 1.4.
Andrew
>From 3fca39549abc2606c7e51420d69dd241a9877a12 Mon Sep 17 00:00:00 2001
From: Andrew Eikum
Date: Mon, 13 Feb 2012 09:47:03 -0600
Subject: winealsa.drv: Optionally load default ALSA device names from the
registry
To: wine-patches
Reply-To: wine-devel ,Andrew Eikum
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="1.7.9"
This is a multi-part message in MIME format.
--1.7.9
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
---
dlls/winealsa.drv/Makefile.in |2 +-
dlls/winealsa.drv/mmdevdrv.c | 38 ++
2 files changed, 31 insertions(+), 9 deletions(-)
--1.7.9
Content-Type: text/x-patch; name="0004-winealsa.drv-Optionally-load-default-ALSA-device-nam.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: inline; filename="0004-winealsa.drv-Optionally-load-default-ALSA-device-nam.patch"
diff --git a/dlls/winealsa.drv/Makefile.in b/dlls/winealsa.drv/Makefile.in
index 336da8b..7086ce6 100644
--- a/dlls/winealsa.drv/Makefile.in
+++ b/dlls/winealsa.drv/Makefile.in
@@ -1,5 +1,5 @@
MODULE= winealsa.drv
-IMPORTS = uuid ole32
+IMPORTS = uuid ole32 advapi32
DELAYIMPORTS = winmm
EXTRALIBS = @ALSALIBS@
diff --git a/dlls/winealsa.drv/mmdevdrv.c b/dlls/winealsa.drv/mmdevdrv.c
index 7621423..c58427a 100644
--- a/dlls/winealsa.drv/mmdevdrv.c
+++ b/dlls/winealsa.drv/mmdevdrv.c
@@ -147,8 +147,7 @@ static CRITICAL_SECTION_DEBUG g_sessions_lock_debug =
static CRITICAL_SECTION g_sessions_lock = { &g_sessions_lock_debug, -1, 0, 0, 0, 0 };
static struct list g_sessions = LIST_INIT(g_sessions);
-static const WCHAR defaultW[] = {'d','e','f','a','u','l','t',0};
-static const char defname[] = "default";
+static const char ALSA_Default[] = "default";
static const IAudioClientVtbl AudioClient_Vtbl;
static const IAudioRenderClientVtbl AudioRenderClient_Vtbl;
@@ -345,28 +344,51 @@ static HRESULT alsa_enum_devices(EDataFlow flow, WCHAR **ids, char **keys,
{
snd_pcm_stream_t stream = (flow == eRender ? SND_PCM_STREAM_PLAYBACK :
SND_PCM_STREAM_CAPTURE);
+static const WCHAR drv_keyW[] = {'S','o','f','t','w','a','r','e','\\',
+'W','i','n','e','\\','D','r','i','v','e','r','s','\\',
+'w','i','n','e','a','l','s','a','.','d','r','v',0};
+static const char ALSAOutputDevice[] = "ALSAOutputDevice";
+static const char ALSAInputDevice[] = "ALSAInputDevice";
+DWORD len;
int err, card;
+HKEY key;
+const char *defname = ALSA_Default;
+char reg_default[64];
-card = -1;
*num = 0;
+if(RegOpenKeyW(HKEY_CURRENT_USER, drv_keyW, &key) == ERROR_SUCCESS){
+DWORD size = sizeof(reg_default);
+const char *value_name = (flow == eRender) ? ALSAOutputDevice : ALSAInputDevice;
+
+if(RegQueryValueExA(key, value_name, 0, NULL,
+(BYTE*)reg_default, &size) == ERROR_SUCCESS){
+defname = reg_default;
+TRACE("Loaded ALSA device from registry: %s\n", defname);
+}
+
+RegCloseKey(key);
+}
+
if(alsa_try_open(defname, stream)){
if(ids && keys){
-*ids = HeapAlloc(GetProcessHeap(), 0, sizeof(defaultW));
-memcpy(*ids, defaultW, sizeof(defaultW));
-*keys = HeapAlloc(GetProcessHeap(), 0, sizeof(defname));
-memcpy(*keys, defname, sizeof(defname));
+len = MultiByteToWideChar(CP_UNIXCP, 0, defname, -1, NULL, 0);
+*ids = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
+MultiByteToWideChar(CP_UNIXCP, 0, defname, -1, *ids, len);
+
+*keys = HeapAlloc(GetProcessHeap(), 0, strlen(defname) + 1);
+memcpy(*keys, defname, strlen(defname) + 1);
}
++*num;
}
+card = -1;
for(err = snd_card_next(&card); card != -1 && err >= 0;
err = snd_card_next(&card)){
char cardpath[64];
char *cardname;
WCHAR *cardnameW;
snd_ctl_t *ctl;
-DWORD len;
sprintf(cardpath, "hw:%u", card);
--1.7.9--
Re: winealsa.drv patch to enum all software devices from .asoundrc
On Sat, Feb 11, 2012 at 06:24:08AM -0800, Chris Robinson wrote: > On Saturday, February 11, 2012 2:05:18 PM Нискородов Серёжа wrote: > > Here is another trouble with snd_ctl_open. Not all devices in alsa > > configuration files have a predefined ctl. Especially defined by user, > > for example I don't always define a ctls for all my devices in > > .asoundconf > > You can use the "hw:" prefix for snd_ctl_open. All you need it for is to get > the card info and the available device indices, which doesn't rely on a > custom > prefix. > > Hmm, actually it seems winealsa already does this enumerating. Except it > hardcodes the plughw: prefix for the device string, uses the card index > instead of the name, and doesn't explicitly specify the parameter names (it > uses "x,y" instead of "CARD=x,DEV=y"). Perhaps I'll see about making some > patches. > Please, I would love to see how someone more familiar with ALSA would do this. My understanding is: snd_ctl_name_hint() and friends: +Includes user-specified, non-hw devices as well as true hw devices, but: -Misses devices without 'hint' configs (Arch had this problem until a few weeks ago, other distros?) -Duplicates devices many times -Requires plughw: or other impossible-to-query prefix snd_card_next() and friends: +Each device listed is listed only once, but: -Misses all user-specified, non-hw devices -Requires plughw: or other impossible-to-query prefix User-specified device list: +Includes all devices a user wants, but: -Really sucks from a UI perspective Each option has huge downsides. I would have guessed that name_hint() was the best choice, but you disagree with that, right? > I'd agree with getting rid of alsa_try_open, unless there's a reason to keep > it. I don't think there's a good reason to prevent enumerating the extra > devices that can't be opened here though. It's status as unavailable/in-use > can be handled later, I think. > The reason was to prevent displaying broken or useless devices. Another way to do it would be to report these devices, but fail to Initialize them with a more interesting error code (DEVICE_INVALIDATED or something) and handle that correctly in dsound, winmm, and the associated test suites (Jorg has a patchset that does most of this). Unfortunately, we'd potentially be presenting broken devices to the user, but I think it's probably better than trying to guess if a device is broken like we do now. Andrew
Re: winealsa.drv patch to enum all software devices from .asoundrc
On Thu, Feb 09, 2012 at 10:08:39PM +0200, Нискородов Серёжа wrote:
> > Thanks, I've been wanting to do this for a while. I've been hesitating
> > because ALSA doesn't document the format of their device names. If a
> > device name contains a colon, is it _always_ a hardware device that
> > will be returned by the snd_card_* family? Who knows...
>
> To check whether the device is a hardware, I can call a function
> snd_pcm_type. But first I need to get handle of pcm device using the
> snd_pcm_open. Okay, but then I have to cancel calling the function
> alsa_try_open and rewrite checking availability of the device to not
> to opening it twice... Or maybe rewrite function alsa_try_open to
> return a snd_pcm_type_t?
> Huh ... I decided to just check for the presence of a colon in the
> name of the device. So it was easier.
>
Yeah, I think it's okay as it is for now.
> > Why do you create stream2? It looks like stream does what you wanted
> > already.
>
> Honestly, I just do not understand the syntax of its assignment. So I
> just copied stream variable from aplay source to.
>
It's the conditional operator in C. See:
<http://en.wikipedia.org/wiki/%3F:#Usage>
> >> + ids[*num] = HeapAlloc(GetProcessHeap(), 0, len *
> >> sizeof(WCHAR));
> >> + if(!ids[*num]){
> >> + return E_OUTOFMEMORY;
> >> + }
> >
> > Doing correct cleanup after HeapAlloc() failure is complicated here.
> > Since things are going to crash badly if HeapAlloc() fails anyway, I
> > think you can just assume success and get rid of the error checking.
>
> Are you sure? This piece of code was taken from function
> alsa_get_card_devices() (mmdevdrv.c), I just copied the way of
> assigning and checking for errors. I think if the check is performed
> there, I also have to fulfill it.
>
The trouble is that to be completely correct, you have to Free the
values you Allocated into previous members of the ids and keys arrays,
as well as the arrays themselves. That means a for-loop over only the
valid members of those arrays, and it turns into quite a lot of code
for an error path that will never be executed anyway.
You should either do it correctly or not at all. Your choice :)
> > If snd_device_name_hint() fails, you should print a WARN message so we
> > can know if something went wrong.
>
> You think this is really necessary? If snd_device_name_hint() fails, I
> just skip of enumeration of software devices. Does it really need to
> print WARN message here?
>
You can use TRACE instead, it doesn't really matter. It's just so we
know what went wrong is someone doesn't get all the devices they
expect. Better to have too much information than too little.
> > One more thing, this messes up the default device selection, which
> > remains hard-coded for the first device. I'd suggest something like
> > the following to add to your patch. It chooses "default" as the
> > default device, or "pulse" if that doesn't exist.
>
> The point is that the device "default" is already listed with the
> function snd_device_name_hint (). So I thought it unnecessary to add a
> device "default" in the first place, and deleted the code. After all,
> if you select in winecfg "(System default)" then in any case, the
> sound goes to the"default" device.Or am I wrong?
>
The word "default" in very overloaded this context :)
The def_index parameter to AUDDRV_GetEndpointIDs() is what determines
which device gets mapped to System Default. We want to set it to the
index of the system's default device. As the code is in Wine now,
index 0 is always ALSA device "default". But with your changes, index
0 is whatever happens to be returned first from
snd_device_name_hint(). On my system, that's "null" which is not what
we want!
My suggested change sets def_index to the index of the ALSA device
"default" (or "pulse" (or, finally, just the first device)) which is
much more likely to be what the user expects.
> > your wine-devel mail was
> > not formatted correctly.
>
> I was trying to Follow all instructions. What is my mistake?
>
I just meant that it wasn't correct for sending to wine-patches.
Basically, just make your changes as a commit in Git, and then use
git-format-patch to create a patch file. You can attach that to your
email and send it.
Take a look at some mails here <http://source.winehq.org/patches/> for
an idea of what your mail (or attachment) should look like when you
send it.
You can also find more information here:
<http://wiki.winehq.org/GitWine>
Andrew
Re: winealsa.drv patch to enum all software devices from .asoundrc
On Tue, Feb 07, 2012 at 11:32:34PM +0200, Нискородов Серёжа wrote:
> Perhaps the code is not so clear and beautiful, but it works for me,
> and maybe someone wants to send it to the repository, or to correct
> and send a corrected.
One more thing, this messes up the default device selection, which
remains hard-coded for the first device. I'd suggest something like
the following to add to your patch. It chooses "default" as the
default device, or "pulse" if that doesn't exist.
@@ -147,9 +147,6 @@ static CRITICAL_SECTION_DEBUG g_sessions_lock_debug =
static CRITICAL_SECTION g_sessions_lock = { &g_sessions_lock_debug, -1, 0, 0,
0, 0 };
static struct list g_sessions = LIST_INIT(g_sessions);
-static const WCHAR defaultW[] = {'d','e','f','a','u','l','t',0};
-static const char defname[] = "default";
-
static const IAudioClientVtbl AudioClient_Vtbl;
static const IAudioRenderClientVtbl AudioRenderClient_Vtbl;
static const IAudioCaptureClientVtbl AudioCaptureClient_Vtbl;
@@ -435,8 +469,6 @@ HRESULT WINAPI AUDDRV_GetEndpointIDs(EDataFlow flow, WCHAR
***ids, char ***keys,
return E_OUTOFMEMORY;
}
-*def_index = 0;
-
hr = alsa_enum_devices(flow, *ids, *keys, num);
if(FAILED(hr)){
int i;
@@ -449,6 +481,18 @@ HRESULT WINAPI AUDDRV_GetEndpointIDs(EDataFlow flow, WCHAR
***ids, char ***keys,
return E_OUTOFMEMORY;
}
+for(*def_index = 0; *def_index < *num; ++*def_index)
+if(!strcmp((*keys)[*def_index], "default"))
+break;
+
+if(*def_index >= *num)
+for(*def_index = 0; *def_index < *num; ++*def_index)
+if(!strcmp((*keys)[*def_index], "pulse"))
+break;
+
+if(*def_index >= *num)
+*def_index = 0;
+
return S_OK;
}
Re: winealsa.drv patch to enum all software devices from .asoundrc
On Tue, Feb 07, 2012 at 11:32:34PM +0200, Нискородов Серёжа wrote:
> I'm not so good at programming. I just was looking for a way to
> make possible play sound from WINE to ALSA software device other than
> "default".
>
> I wrote a patch that allows you to select the ALSA software device
> from control panel, along with the hardware devices.
>
Thanks, I've been wanting to do this for a while. I've been hesitating
because ALSA doesn't document the format of their device names. If a
device name contains a colon, is it _always_ a hardware device that
will be returned by the snd_card_* family? Who knows... In any case,
your patch takes the conservative approach, so I think it's a good
start. We can fine-tune it later if we run into problems.
Your patch is mostly correct, but a couple of small things need to be
fixed.
> +void **hints, **n;
> +snd_pcm_stream_t stream2 = SND_PCM_STREAM_PLAYBACK;
> +char *name, *io;
> +const char *filter;
> +WCHAR *nameW;
> +DWORD len;
Your variable declarations need to happen at the start of the
function, where the rest are located.
You can probably come up with a better name than "n" ("hint"? "cur"?).
Why do you create stream2? It looks like stream does what you wanted
already.
> +if(strstr(name, ":") != NULL )
> +goto __end;
> +if (io != NULL && strcmp(io, filter) != 0)
> +goto __end;
Instead of using goto, just use regular if/else. You may have to
indent everything once more, but that's fine.
> +ids[*num] = HeapAlloc(GetProcessHeap(), 0, len *
> sizeof(WCHAR));
> +if(!ids[*num]){
> +return E_OUTOFMEMORY;
> +}
Doing correct cleanup after HeapAlloc() failure is complicated here.
Since things are going to crash badly if HeapAlloc() fails anyway, I
think you can just assume success and get rid of the error checking.
> +}
> +++*num;
> +}
Indentation is wrong.
> +if (name != NULL)
> +free(name);
free() checks for NULL anyway, so the if() is not needed.
If snd_device_name_hint() fails, you should print a WARN message so we
can know if something went wrong.
And finally, you should use snd_device_name_free_hint(hints) to clean
up those resources.
> Perhaps the code is not so clear and beautiful, but it works for me,
> and maybe someone wants to send it to the repository, or to correct
> and send a corrected.
It's not bad at all! After fixing the little stuff above, you can send
it to wine-patches yourself. Please read
<http://wiki.winehq.org/SubmittingPatches>; your wine-devel mail was
not formatted correctly.
Andrew
Re: winmm: TRACE unhandled messages
On Wed, Feb 08, 2012 at 11:22:42AM +0100, joerg-cyril.hoe...@t-systems.com wrote: > Your rewrite no more maps WOD_XYZ* messages to waveOut functions. > I believe that it is possible that an old app entirely uses the generic > SendDriverMessage function > (esp. in the mciavi/msvfw area, cf. ICSendMessage) instead of the specific > wave/midi* ones. > Can you be sure that this is not supported by native, esp. w9x? > Good question. All of the following is on my Win 7 VM. I tested both with no compatibility and with WinXP SP3 compatibility. I tried using OpenDriver with "wave", "wavemapper", "wdmaud.drv", and "msacm32.drv" (last two found in Win 7 registry), all of which return NULL (ie, error). So I think SendDriverMessage() is unlikely to be interesting. I wasn't able to get any useful results with WODM_* or WOD_* through waveOutMessage. Interestingly, WOD_OPEN returns NOTSUPPORTED, while WODM_OPEN crashes. I tried passing various combinations of WAVEOPENDESC, MMDRV_MESSAGE_PARAMS (not in the PSDK or Wine's include), and NULL to the function, but everything crashes. It's possible there's something useful there, but I can't figure out how to get at it. Using the simpler case of WODM_GETNUMDEVS also failed in every case. > If Wine does not map these, all we'll get to "hear" in AppDB is that attentive > people miss sound or music that they remember listening to 10 years ago on > native. > Well, hopefully they would also file bugs :) Andrew
Re: winecoreaudio.drv: Improve underrun handling
On Mon, Jan 30, 2012 at 06:52:36PM +0100, joerg-cyril.hoe...@t-systems.com wrote: > Andre Eikum wrote: > >>> +sc = AudioQueueEnqueueBufferWithParameters(This->aqueue, > >>> +This->public_buffer, 0, NULL, 0, 0, 0, NULL, > >>> + &req_time, &start_time); > >> NULL=ASAP is exactly what we want. We don't want ALSA/dmix behavior, > >> where it'll silently skip over late frames in order to catch up (dunno > >> if MacOSX does this but in my eyes that doesn't matter). > >The AudioQueue API does do this > > What "this" do you mean exactly? > a) like dmix, silently skip over megabytes of submitted frames >should they happen to be late w.r.t. wall time? > b) Play any submitted frames ASAP to the speaker? >You said in #29585 that OSS4 does this. > > What mmdevapi needs is b). It does not appear to care about underruns > and has no dmix-like notion of trying to catch up. > Yes. AudioQueue does (a), which I'm trying to work around by telling the queue to play the buffer at any time after the queue's current time when we detect an underrun (although detecting when the queue is empty needs some work, as Ken pointed out). In ALSA, we use snd_pcm_recover() after underruns, but AudioQueue doesn't seem to have an analogous function that works well. Andrew
Re: winecoreaudio.drv: Improve underrun handling
On Mon, Jan 30, 2012 at 11:03:52AM +0100, joerg-cyril.hoe...@t-systems.com wrote: > +sc = AudioQueueEnqueueBufferWithParameters(This->aqueue, > +This->public_buffer, 0, NULL, 0, 0, 0, NULL, &req_time, > &start_time); > I advise against setting req_time. This can only fragilize the > system. What if req_time is miscomputed or there's a delay? > NULL=ASAP is exactly what we want. We don't want ALSA/dmix behavior, > where it'll silently skip over late frames in order to catch up > (dunno if MacOSX does this but in my eyes that doesn't matter). The AudioQueue API does do this, and that's the behavior I was trying to work around. See bug 29602. Other options like AudioQueueReset() or AudioQueueFlush() don't seem to do what we want, so this is the best solution I found. What do you mean by it doesn't matter? > >if(list_count(...) == 0) > I once wrote a patch to turn such a pattern into if(list_empty(...)) > Oops, I forgot that existed. I'll fix that in the next version. Andrew
Re: [PATCH 2/3] winecoreaudio: Implement the lock-free callback design.
On Mon, Jan 30, 2012 at 11:37:17AM +0100, joerg-cyril.hoe...@t-systems.com wrote: > Fixes bug #29657 > While I like the idea of this patch and how you implemented it, I wonder if it's appropriate for code freeze. The main problem in that bug is that the CA driver can deadlock, or release memory out of order. When working on this patch, were you able to identify specifically where the contention is? The idea of the InTransition state was to handle the lock release in mid-call. Can we add a check for that state in the callbacks to solve this problem less intrusively? The first patch seems fine. Andrew
Re: [PATCH 2/2] winmm: Restore useful MMSYSERR_* codes at open time.
On Fri, Jan 27, 2012 at 02:02:22PM +0100, joerg-cyril.hoe...@t-systems.com wrote: > +case S_FALSE: /* as in IsFormatSupported */ > +case AUDCLNT_E_UNSUPPORTED_FORMAT: > +return WAVERR_BADFORMAT; I think I'd rather special-case how we handle IsFormatSupported, rather than put S_FALSE->BADFORMAT into the generic conversion. Andrew
