Re: [PATCH 1/4] d3drm: Add stubs for IDirect3DRMFrame interface. (v4)
On 17 June 2016 at 09:38, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
v4: Replaced GetVisuals, GetParent and GetTransform with thunks to version 2.
v3: Added version 1->3 thunks for only those frame1 methods that have tests (see test_Frame). The rest will be stubs returning E_NOTIMPL. Also avoid the use of QI in Add/DeleteChild.
Doesn't that potentially break currently working applications? And while having tests is great, I'm not sure I agree with the premise that it's better to return E_NOTIMPL than to forward to an implementation that may have subtle differences but probably doesn't in most cases.
+static HRESULT WINAPI d3drm_frame1_QueryInterface(IDirect3DRMFrame *iface, REFIID riid, void **out) +{ + struct d3drm_frame *frame = impl_from_IDirect3DRMFrame(iface); + + TRACE("iface %p, riid %s, out %p.\n", iface, debugstr_guid(riid), out); + + return IDirect3DRMFrame3_QueryInterface(&frame->IDirect3DRMFrame3_iface, riid, out); +} This may have come up before, but any reason you can't call d3drm_frame3_QueryInterface() directly here?
On Fri, Jun 17, 2016 at 6:44 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 17 June 2016 at 09:38, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
v4: Replaced GetVisuals, GetParent and GetTransform with thunks to version 2.
v3: Added version 1->3 thunks for only those frame1 methods that have tests (see test_Frame). The rest will be stubs returning E_NOTIMPL. Also avoid the use of QI in Add/DeleteChild.
Doesn't that potentially break currently working applications? And while having tests is great, I'm not sure I agree with the premise that it's better to return E_NOTIMPL than to forward to an implementation that may have subtle differences but probably doesn't in most cases.
I don't disagree with you. I just want an acceptable patch for this as I'll need it for implementing viewports. Also a lot of version 2 stuff uses version 1 frames as well, which makes it more important. I've CC'd Stefan in this mail, hopefully we can agree on something which works for all.
+static HRESULT WINAPI d3drm_frame1_QueryInterface(IDirect3DRMFrame *iface, REFIID riid, void **out) +{ + struct d3drm_frame *frame = impl_from_IDirect3DRMFrame(iface); + + TRACE("iface %p, riid %s, out %p.\n", iface, debugstr_guid(riid), out); + + return IDirect3DRMFrame3_QueryInterface(&frame->IDirect3DRMFrame3_iface, riid, out); +} This may have come up before, but any reason you can't call d3drm_frame3_QueryInterface() directly here?
I could probably put these stubs after version 3, but wouldn't that make the version order in the file 2->3->1? Maybe a separate patch could rearrange it to 3->2->1. Maybe we could do that after these stubs are added. Cheers, Aaryaman
On 17 June 2016 at 15:40, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
On Fri, Jun 17, 2016 at 6:44 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
This may have come up before, but any reason you can't call d3drm_frame3_QueryInterface() directly here?
I could probably put these stubs after version 3, but wouldn't that make the version order in the file 2->3->1? Maybe a separate patch could rearrange it to 3->2->1. Maybe we could do that after these stubs are added.
I'd make the order d3drm_frame3_QueryInterface() d3drm_frame2_QueryInterface() d3drm_frame1_QueryInterface() d3drm_frame3_AddRef() d3drm_frame2_AddRef() d3drm_frame1_AddRef() etc. I'd don't care so much if you move the version 2 functions in the same patch that introduces the version 1 functions, although I think it makes sense to do that in a separate patch.
On Fri, Jun 17, 2016 at 7:23 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 17 June 2016 at 15:40, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
On Fri, Jun 17, 2016 at 6:44 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
This may have come up before, but any reason you can't call d3drm_frame3_QueryInterface() directly here?
I could probably put these stubs after version 3, but wouldn't that make the version order in the file 2->3->1? Maybe a separate patch could rearrange it to 3->2->1. Maybe we could do that after these stubs are added.
I'd make the order
d3drm_frame3_QueryInterface() d3drm_frame2_QueryInterface() d3drm_frame1_QueryInterface() d3drm_frame3_AddRef() d3drm_frame2_AddRef() d3drm_frame1_AddRef() etc.
I'd don't care so much if you move the version 2 functions in the same patch that introduces the version 1 functions, although I think it makes sense to do that in a separate patch.
Right, will get right on it. I'll first write a patch to rearrange it and rebase these patches on top of it. Please review the other patches in the mean time if possible. Thanks for the review! Cheers, Aaryaman
Am 17.06.2016 um 14:40 schrieb Aaryaman Vasishta <jem456.vasishta(a)gmail.com>:
Doesn't that potentially break currently working applications? And while having tests is great, I'm not sure I agree with the premise that it's better to return E_NOTIMPL than to forward to an implementation that may have subtle differences but probably doesn't in most cases.
I don't disagree with you. I just want an acceptable patch for this as I'll need it for implementing viewports. Also a lot of version 2 stuff uses version 1 frames as well, which makes it more important. I've CC'd Stefan in this mail, hopefully we can agree on something which works for all. I am fine with imperfect / possibly broken thunks. What I want to avoid is wasting too much time on possible corner cases for a thunk that thunks to an E_NOTIMPL or barely implemented function. I guess I forgot about the possibility that the existing Frame code runs any real application :-\ .
On a deeper look, it seems only GetTransform is such a thunk that doesn't have tests for version 1. I'll add this thunk and resend the patch, after the rearrangement changes. Cheers, Aaryaman On Fri, Jun 17, 2016 at 11:44 PM, Stefan Dösinger <stefandoesinger(a)gmail.com
wrote:
Am 17.06.2016 um 14:40 schrieb Aaryaman Vasishta < jem456.vasishta(a)gmail.com>:
Doesn't that potentially break currently working applications? And while having tests is great, I'm not sure I agree with the premise that it's better to return E_NOTIMPL than to forward to an implementation that may have subtle differences but probably doesn't in most cases.
I don't disagree with you. I just want an acceptable patch for this as I'll need it for implementing viewports. Also a lot of version 2 stuff uses version 1 frames as well, which makes it more important. I've CC'd Stefan in this mail, hopefully we can agree on something which works for all. I am fine with imperfect / possibly broken thunks. What I want to avoid is wasting too much time on possible corner cases for a thunk that thunks to an E_NOTIMPL or barely implemented function. I guess I forgot about the possibility that the existing Frame code runs any real application :-\ .
participants (3)
-
Aaryaman Vasishta -
Henri Verbeet -
Stefan Dösinger