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

Re: Definition (or not) of IDXGIResource methods

by Henri Verbeet

On 30 June 2017 at 19:54, Ben "Root" Anderson <roothorick(a)gmail.com> wrote: > I've been studying the DXGI code with an eye for implementing the > DirectX-side shared resource functionality so that an application > would hopefully be able to share Direct3D textures with a Vulkan > instance, and what I'm seeing has me rather confused. > > IDXGIResource appears to be declared, with a number of methods, in > include/dxgi.idl. I'm not clear on what exactly an IDL file is, but my > impression is it's similar to a specfile, except for headers of C++ > interfaces. In any case, none of the methods declared appear to have > any definition anywhere in Wine. The logical conclusion would be > they're automatically generated stubs. > > But that doesn't make sense, because the DXGI test at > dlls/dxgi/tests/device.c calls IDXGIResource_GetUsage and appears to > expect it to succeed, Actually, no, it doesn't. The IDXGISwapChain_GetBuffer() call in test_swapchain_parameters() that's supposed to return the IDXGIResource interface is marked todo_wine. I.e., it's expected to fail currently. The IDXGIResource interface is indeed currently unimplemented in Wine.

June 30, 2017

Re: [PATCH 03/10 v4] gdiplus: Add functions for managing metafile objects id

by Piotr Caban

> On 30 Jun 2017, at 22:24, Vincent Povirk <madewokherd(a)gmail.com> wrote: > > This seems like overkill to me? > > I don't see the need for per-object refcounting. Every drawing > operation that uses an object is just going to end with a series of > releases, and after it returns all refcounts should be 0. > > + /* TODO: object id should be released on object dispose */ > I don't think this approach is possible. It would require every object > to be aware of which metafiles are using it, and every operation that > modifies the object would have to release them. Bitmap objects can > refer to an application buffer that could be modified without any call > into gdiplus. (Or, Bitmaps could be modified by an ImageAttributes, in > which case we can't reuse them except with an identical > ImageAttributes.) > > If we wanted to reuse Bitmaps, we'd have to compare the data. For > other large types like Paths, maybe keeping an id to detect whether it > changed would make sense. For types of bounded size, it probably would > be easier to store a complete copy. Currently the implementation only overwrites ids in the same way as native does. It’s there to make sure we never run out of IDs. I think it’s nice that the functions are failsafe. It’s also closer to native behaviour. I think that everything else can be figured out later. The comment is based on observation of native dll behaviour. I’ve checked what happens when new bitmaps are created and drawn to metafile in a loop. All of the bitmaps contained exactly the same data. In this case the IDs were not freed unless we run out of free IDs. After adding GdiDisposeImage call to the loop the IDs got released. I think it makes the comment valid even if we never implement this behaviour. Thanks, Piotr

June 30, 2017

Re: [PATCH 03/10 v4] gdiplus: Add functions for managing metafile objects id

by Vincent Povirk

This seems like overkill to me? I don't see the need for per-object refcounting. Every drawing operation that uses an object is just going to end with a series of releases, and after it returns all refcounts should be 0. + /* TODO: object id should be released on object dispose */ I don't think this approach is possible. It would require every object to be aware of which metafiles are using it, and every operation that modifies the object would have to release them. Bitmap objects can refer to an application buffer that could be modified without any call into gdiplus. (Or, Bitmaps could be modified by an ImageAttributes, in which case we can't reuse them except with an identical ImageAttributes.) If we wanted to reuse Bitmaps, we'd have to compare the data. For other large types like Paths, maybe keeping an id to detect whether it changed would make sense. For types of bounded size, it probably would be easier to store a complete copy.

June 30, 2017

Definition (or not) of IDXGIResource methods

by Ben "Root" Anderson

I've been studying the DXGI code with an eye for implementing the DirectX-side shared resource functionality so that an application would hopefully be able to share Direct3D textures with a Vulkan instance, and what I'm seeing has me rather confused. IDXGIResource appears to be declared, with a number of methods, in include/dxgi.idl. I'm not clear on what exactly an IDL file is, but my impression is it's similar to a specfile, except for headers of C++ interfaces. In any case, none of the methods declared appear to have any definition anywhere in Wine. The logical conclusion would be they're automatically generated stubs. But that doesn't make sense, because the DXGI test at dlls/dxgi/tests/device.c calls IDXGIResource_GetUsage and appears to expect it to succeed, implying that it's implemented *somewhere*. What am I missing?

June 30, 2017

Re: winedump: Add initial support for handling EMF+ records

by Piotr Caban

On 06/30/17 17:50, Dmitry Timoshkov wrote: > Piotr Caban <piotr(a)codeweavers.com> wrote: > >> #define EMRCASE(x) case x: printf("%-20s %08x\n", #x, length); break >> +#define EMRPLUSCASE(x) case x: printf(" %-20s %04x %08x\n", #x, header->Flags, header->Size); break; > > ';' at the end is redundant. EMRCASE macro intentionally omits it. > >> + /* Hand EMF+ records */ >> + if (length >= 16 && !memcmp((char*)PRD(offset + 12, sizeof(unsigned int)), "EMF+", 4)) > > A typo in the comment. Thanks, I'll send a fixed version.

June 30, 2017

Re: winedump: Add initial support for handling EMF+ records

by Dmitry Timoshkov

Piotr Caban <piotr(a)codeweavers.com> wrote: > #define EMRCASE(x) case x: printf("%-20s %08x\n", #x, length); break > +#define EMRPLUSCASE(x) case x: printf(" %-20s %04x %08x\n", #x, header->Flags, header->Size); break; ';' at the end is redundant. EMRCASE macro intentionally omits it. > + /* Hand EMF+ records */ > + if (length >= 16 && !memcmp((char*)PRD(offset + 12, sizeof(unsigned int)), "EMF+", 4)) A typo in the comment. -- Dmitry.

June 30, 2017

Re: [PATCH 1/9 v3] gdiplus: Add partial support for GdipDrawImagePointsRect on metafile

by Piotr Caban

On 06/30/17 16:46, Dmitry Timoshkov wrote: > Piotr Caban <piotr.caban(a)gmail.com> wrote: > >>> Even internally gdiplus shouldn't do this kind of hacks IMO, if there is >>> a need for a shortcut for a particular encoder class id it's better to >>> add a helper. Have a look at the encoder/decoder tests how it's supposed >>> to look like. >> I've done it this way because I don't think we should go through >> enumerating all supported formats internally. > > Why not? Mainly because it unnecessarily complicates the code. Also I don't see any advantages from doing it this way.

June 30, 2017

Re: [PATCH 1/9 v3] gdiplus: Add partial support for GdipDrawImagePointsRect on metafile

by Dmitry Timoshkov

Piotr Caban <piotr.caban(a)gmail.com> wrote: > > Even internally gdiplus shouldn't do this kind of hacks IMO, if there is > > a need for a shortcut for a particular encoder class id it's better to > > add a helper. Have a look at the encoder/decoder tests how it's supposed > > to look like. > I've done it this way because I don't think we should go through > enumerating all supported formats internally. Why not? > How about using > encode_image_png function directly? I personally don't like it, this is not a performance critical code. While looking at this I noticed that 'clsid' parameter in encode_image_func() is unused, that's probably a leftover from previous implementation. -- Dmitry.

June 30, 2017

Re: [PATCH 1/9 v3] gdiplus: Add partial support for GdipDrawImagePointsRect on metafile

by Piotr Caban

On 06/30/17 16:14, Dmitry Timoshkov wrote: > Piotr Caban <piotr(a)codeweavers.com> wrote: > >> +static GpStatus METAFILE_CreateCompressedImageStream(GpImage *image, IStream **stream, DWORD *size) >> +{ >> + static const CLSID png_clsid = { 0x557cf406, 0x1a04, 0x11d3, >> + { 0x9a, 0x73, 0x0, 0x0, 0xf8, 0x1e, 0xf3, 0x2e } }; >> + LARGE_INTEGER zero; >> + STATSTG statstg; >> + GpStatus stat; >> + HRESULT hr; >> + >> + *size = 0; >> + >> + hr = CreateStreamOnHGlobal(NULL, TRUE, stream); >> + if (FAILED(hr)) return hresult_to_status(hr); >> + >> + stat = GdipSaveImageToStream(image, *stream, &png_clsid, NULL); > > Even internally gdiplus shouldn't do this kind of hacks IMO, if there is > a need for a shortcut for a particular encoder class id it's better to > add a helper. Have a look at the encoder/decoder tests how it's supposed > to look like. I've done it this way because I don't think we should go through enumerating all supported formats internally. How about using encode_image_png function directly? Thanks, Piotr

June 30, 2017

Re: [PATCH 3/9 v3] gdiplus: Add support for ImageAttributes when drawing image to metafile

by Piotr Caban

On 06/30/17 16:11, Dmitry Timoshkov wrote: > Piotr Caban <piotr(a)codeweavers.com> wrote: > >> @@ -259,7 +278,7 @@ static DWORD METAFILE_AddObjectId(GpMetafile *metafile) >> id = 0; >> for (i=1; i<sizeof(metafile->objects_table)/sizeof(metafile->objects_table[0]); i++) >> { >> - if (metafile->objects_table[id] < metafile->objects_table[i]) >> + if (metafile->objects_table[id] > metafile->objects_table[i]) > > This looks strange to change this right in the next patch. I've made a mistake while rebasing the patches. Thanks for catching it. Thanks, Piotr

June 30, 2017