June 2012 - Wine-Devel - list.winehq.org

Re: windowscodecs: Create a IWICMetadataReader when loading a TIFF frame.

by Dmitry Timoshkov

Vincent Povirk <madewokherd(a)gmail.com> wrote: > >> We can't use that stream because we provide separate seek and > >> read/write methods to libtiff, and libtiff may rely on the stream's > >> position to not change between operations. > > > > libtiff doesn't rely on stream positions, and my use of stream matches > > MSDN guidelines on writing a WIC enabled codec. > > We should provide the stream interface to libtiff as documented and > not rely on implementation details. > > Even without libtiff, you will end up using the same stream for > multiple metadata handlers, each of which could end up using it later, > unless you specify WICPersistOptionNoCacheStream. That's not the case of an IFD metadata reader, it creates all the items on load, so creating multiple metadata readers from the same stream won't hurt. > >> Your GetReaderByIndex method should check whether > >> This->metadata_reader has been set and return an error if not. > > > > No, that's not an error. TIFF always provides metadata since IFD is > > the core of TIFF. If a metadata reader can't be created it's OK to > > return 0 metadata items. > > That's fine, but your check in GetCount suggests that having no > metadata reader is a valid state, and GetReaderByIndex(0) will crash > in that case. If it's not valid, you shouldn't need to test for it in > GetCount. That's a valid point, sorry, I misread your comment first time. > Also, I just noticed that you're calling create_metadata_reader > unconditionally from QueryInterface, and not checking in the process > whether the reader has already been created. That will leak if QI is > called multiple times. > > I'm not sure doing real work inside QueryInterface is a good idea. > It's pretty surprising. > > I think it would make more sense to do this when a method on > IWICMetadataBlockReader is first called, or in Initialize if > WICDecodeMetadataCacheOnLoad is specified (though doing it in > Initialize would require creating the frame objects then as well, and > probably combining their refcounts with the parent). MSDN suggests different ways of creating a metadata reader: 1. when creating a IWICBitmapFrameDecode 2. when creating a block reader 3. inside of GetReaderByIndex It's all up to an implementor. I decided to that at #1. Each instance of IWICBitmapFrameDecode has its own metadata reader, I don't see how it could be created twice or leaked. -- Dmitry.

June 18, 2012

Re: [PATCH] winepulse.drv: Add PulseAudio driver

by Andrew Eikum

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

June 18, 2012

Re: atl80: prefer native, if present

by Alexandre Julliard

Daniel Jelinski <djelinski1(a)gmail.com> writes: > 2012/6/18, Alexandre Julliard <julliard(a)winehq.org>: >> There's no reason to do that, most of the dll is already implemented. > > It hides at least bug [1] from the end user, since the application > comes with a bundled atl80 and works fine with it. It would probably > also hide [2], since that application was reported to work under > wine-1.4, so probably installs atl80 as well. Leaving it as-is means > that users will need to set atl80 to native in order to get these > applications to run properly, at least for the time being. We want to fix bugs, not hide them. -- Alexandre Julliard julliard(a)winehq.org

June 18, 2012

Re: windowscodecs: Create a IWICMetadataReader when loading a TIFF frame.

by Vincent Povirk

>> > + hr = IWICPersistStream_LoadEx(persist, This->parent->stream, NULL, persist_options); >> > + if (FAILED(hr)) >> > + ERR("IWICPersistStream_LoadEx error %#x\n", hr); >> >> We can't use that stream because we provide separate seek and >> read/write methods to libtiff, and libtiff may rely on the stream's >> position to not change between operations. > > libtiff doesn't rely on stream positions, and my use of stream matches > MSDN guidelines on writing a WIC enabled codec. We should provide the stream interface to libtiff as documented and not rely on implementation details. Even without libtiff, you will end up using the same stream for multiple metadata handlers, each of which could end up using it later, unless you specify WICPersistOptionNoCacheStream. >> Your GetReaderByIndex method should check whether >> This->metadata_reader has been set and return an error if not. > > No, that's not an error. TIFF always provides metadata since IFD is > the core of TIFF. If a metadata reader can't be created it's OK to > return 0 metadata items. That's fine, but your check in GetCount suggests that having no metadata reader is a valid state, and GetReaderByIndex(0) will crash in that case. If it's not valid, you shouldn't need to test for it in GetCount. Also, I just noticed that you're calling create_metadata_reader unconditionally from QueryInterface, and not checking in the process whether the reader has already been created. That will leak if QI is called multiple times. I'm not sure doing real work inside QueryInterface is a good idea. It's pretty surprising. I think it would make more sense to do this when a method on IWICMetadataBlockReader is first called, or in Initialize if WICDecodeMetadataCacheOnLoad is specified (though doing it in Initialize would require creating the frame objects then as well, and probably combining their refcounts with the parent).

June 18, 2012

Re: windowscodecs: Create a IWICMetadataReader when loading a TIFF frame.

by Dmitry Timoshkov

Vincent Povirk <madewokherd(a)gmail.com> wrote: > > + hr = IWICPersistStream_LoadEx(persist, This->parent->stream, NULL, persist_options); > > + if (FAILED(hr)) > > + ERR("IWICPersistStream_LoadEx error %#x\n", hr); > > We can't use that stream because we provide separate seek and > read/write methods to libtiff, and libtiff may rely on the stream's > position to not change between operations. libtiff doesn't rely on stream positions, and my use of stream matches MSDN guidelines on writing a WIC enabled codec. > This illustrates a design flaw in WIC. Normally, we would just clone > the stream, but IWICStream doesn't implement the Clone method. I think > we are probably supposed to create a new IWICStream based on the given > stream, but the resulting stream won't be thread-safe with ours. I > think for now it's OK to re-use this stream as long as you seek it > back to where it was when you're done, but we may have to change that > later. There is no need to seek the stream back. > Also, LoadEx should (but does not yet) save the stream so it can > create an IWICFastMetadataEncoder. If you re-use the bitmap decoder's > stream, you should also use the WICPersistOptionNoCacheStream flag > when you create the metadata reader to make sure it won't do that. > > Your GetReaderByIndex method should check whether > This->metadata_reader has been set and return an error if not. No, that's not an error. TIFF always provides metadata since IFD is the core of TIFF. If a metadata reader can't be created it's OK to return 0 metadata items. -- Dmitry.

June 18, 2012

Re: windowscodecs: Create a IWICMetadataReader when loading a TIFF frame.

by Vincent Povirk

> + hr = IWICPersistStream_LoadEx(persist, This->parent->stream, NULL, persist_options); > + if (FAILED(hr)) > + ERR("IWICPersistStream_LoadEx error %#x\n", hr); We can't use that stream because we provide separate seek and read/write methods to libtiff, and libtiff may rely on the stream's position to not change between operations. This illustrates a design flaw in WIC. Normally, we would just clone the stream, but IWICStream doesn't implement the Clone method. I think we are probably supposed to create a new IWICStream based on the given stream, but the resulting stream won't be thread-safe with ours. I think for now it's OK to re-use this stream as long as you seek it back to where it was when you're done, but we may have to change that later. Also, LoadEx should (but does not yet) save the stream so it can create an IWICFastMetadataEncoder. If you re-use the bitmap decoder's stream, you should also use the WICPersistOptionNoCacheStream flag when you create the metadata reader to make sure it won't do that. Your GetReaderByIndex method should check whether This->metadata_reader has been set and return an error if not. Otherwise, the patch looks good to me.

June 18, 2012

Re: atl80: prefer native, if present

by Daniel Jelinski

2012/6/18, Alexandre Julliard <julliard(a)winehq.org>: > Daniel Jelinski <djelinski1(a)gmail.com> writes: > >> From 7243b3594c9c0b13770ede1ab32d4dd1329b1dd4 Mon Sep 17 00:00:00 2001 >> From: Daniel Jelinski <djelinski1(a)gmail.com> >> Date: Sun, 17 Jun 2012 17:37:15 +0200 >> Subject: atl80: prefer native, if present >> >> --- >> dlls/atl80/atl80.c | 11 +++++++++++ >> 1 files changed, 11 insertions(+), 0 deletions(-) > > There's no reason to do that, most of the dll is already implemented. It hides at least bug [1] from the end user, since the application comes with a bundled atl80 and works fine with it. It would probably also hide [2], since that application was reported to work under wine-1.4, so probably installs atl80 as well. Leaving it as-is means that users will need to set atl80 to native in order to get these applications to run properly, at least for the time being. Regards, Daniel [1] http://bugs.winehq.org/show_bug.cgi?id=30444 [2] http://bugs.winehq.org/show_bug.cgi?id=30906

June 18, 2012

msi:RECORD_StreamFromFile bug, help needed

by robert.van.herk＠serioustoys.com

Hi all, I think I found a msi.dll, record.c, RECORD_StreamFromFile. This function calls GlobalAlloc, with a file size as an argument. However, I believe that this can easily fail (which it does in my case) when the file size is too large. In my case, my msi builder (WiX) calls MsiRecordSetStreamW, which calls static function RECORD_StreamFromFile. WiX calls MsiRecordSetStreamW on some file that seems to be 0x11b2acc5 bytes (296 megs) large. Trying to allocate the block of 296 megs causes the error: 036:Call msi.MsiRecordSetStreamW(00000003,00000002,0cc8078c L"C:\\users\\robert\\Temp\\3c5c43d5\\1bfe39fa\\#product.cab") ret=009a4c3a 0036:Call KERNEL32.CreateFileW(0cc8078c L"C:\\users\\robert\\Temp\\3c5c43d5\\1bfe39fa\\#product.cab",80000000,00000001,00000000,00000003,00000000,00000000) ret=202868cf 0036:Ret KERNEL32.CreateFileW() retval=00000100 ret=202868cf 0036:Call KERNEL32.GetFileSize(00000100,0033ef58) ret=202868ef 0036:Ret KERNEL32.GetFileSize() retval=11b2acc5 ret=202868ef 0036:Call KERNEL32.GlobalAlloc(00000000,11b2acc5) ret=202869fb 0036:Ret KERNEL32.GlobalAlloc() retval=00000000 ret=202869fb <--- fail I believe the call to GlobalAlloc stems from RECORD_StreamFromFile. This tries to copy the full contents of the file in memory, and then create an IStream from that memory using CreateStreamOnHGlobal: /* read the data in a file into an IStream */ static UINT RECORD_StreamFromFile(LPCWSTR szFile, IStream **pstm) { ... handle = CreateFileW(szFile, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, NULL); ... sz = GetFileSize(handle, &szHighWord); if( sz != INVALID_FILE_SIZE && szHig sz = GetFileSize(handle, &szHighWord); if( sz != INVALID_FILE_SIZE && szHighWord == 0 ) { hGlob = GlobalAlloc(GMEM_FIXED, sz); // <-- Fails in my case, because sz == 296 megs if( hGlob ) { BOOL r = ReadFile(handle, hGlob, sz, &read, NULL); if( !r ) { GlobalFree(hGlob); hGlob = 0; } } CloseHandle(handle); if( !hGlob ) return ERROR_FUNCTION_FAILED; // <-- hence, this error is raised. hr = CreateStreamOnHGlobal(hGlob, TRUE, pstm); ... } I want to fix this. I guess that apart from that the allocation fails, I have the feeling that copying this entire 296 megs into main memory is anyway a bit heavy, just for creating a stream. So I thought a better solution would be just to create a stream from the file. My first guess would be to use the SHCreateStreamOnFile function: HRESULT SHCreateStreamOnFile( __in LPCTSTR pszFile, __in DWORD grfMode, __out IStream **ppstm ); But then I was thinking: this seems like such an obvious thing to do, that it is almost suspicious. Was there any reason for copying the file contents to main memory and then create a memory stream? For instance, was the intention to have a non-mutable copy in memory, in case the backing file would be altered later on? I guess if that's the case, I could just create a copy of the original file in a temp dir, and mark it as STGM_DELETEONRELEASE. Before I wildly attempt a fix though, I would appreciate it if someone had an idea here... Kind regards, Robert van Herk

June 18, 2012

Re: [PATCH] winepulse.drv: Add PulseAudio driver

by Darryl Slack

Well after fighting with pulseaudio after all this time I have tried this patch with wine 1.5.6 with a Presonus StudioLive 24.4.2 Fire-wire controller within wine, to control software known as Virtual DJ with timecoded vinyl records. With Andrew's patch, there are stalled buffer errors, and I wasn't able to measure latency very well because the controller just kept on reporting 'stalled' I did also try martaans driver, which was added to wine, and latency was much lower, and the controller worked well within Linux, in fact the first time i've actually been able to use it with pulseaudio. I was measuring latency much lower than I expected, 2-4ms. With Andrew's Driver I was not even able to play a timecode from start to finish without 'stalled' buffers. Please Take the time to QA this driver for things just other than 'cos we have it' and consider Martaan's driver as well, because there is a massive improvement in latency over Andrews, and applications requiring fire-wire work much nicer with pulseaudio. My Wine build is 1.5.4, Gentoo 3.4.2-r1. Thanks Darryl

June 18, 2012

Re: mshtml: Implement IHTMLScriptElement_put_src

by Jacek Caban

André, On 06/15/12 23:45, André Hentschel wrote: > --- > dlls/mshtml/htmlscript.c | 14 ++++++++++++-- > dlls/mshtml/tests/dom.c | 6 ++++++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/dlls/mshtml/htmlscript.c b/dlls/mshtml/htmlscript.c > index 376db7b..1714cfe 100644 > --- a/dlls/mshtml/htmlscript.c > +++ b/dlls/mshtml/htmlscript.c > @@ -101,8 +101,18 @@ static HRESULT WINAPI HTMLScriptElement_Invoke(IHTMLScriptElement *iface, DISPID > static HRESULT WINAPI HTMLScriptElement_put_src(IHTMLScriptElement *iface, BSTR v) > { > HTMLScriptElement *This = impl_from_IHTMLScriptElement(iface); > - FIXME("(%p)->(%s)\n", This, debugstr_w(v)); > - return E_NOTIMPL; > + nsAString src_str; > + nsresult nsres; > + > + TRACE("(%p)->(%s)\n", This, debugstr_w(v)); > + > + nsAString_Init(&src_str, v); > + nsres = nsIDOMHTMLScriptElement_SetSrc(This->nsscript, &src_str); > + if (NS_FAILED(nsres)) > + ERR("SetSrc failed: %08x\n", nsres); > + nsAString_Finish (&src_str); > + > + return S_OK; > } > Unfortunately it's not so easy. That won't work this way. Jacek

June 18, 2012