[vlc-devel] [PATCH 2/8] libvlc: have the D3D11 callback give the ID3D11RenderTargetView to use per plane
Steve Lhomme
robux4 at ycbcr.xyz
Thu Apr 1 04:47:01 UTC 2021
It is long because I didn't find any shorter way to say there is an important change in the expected behavior in one particular use case of libvlc. The change you proposed is too generic IMO. I prefer commit titles to be meaningful so I don't have to dig in each commit message to find exactly how and why things have changed. That to me is the extra distraction I don't want.
If we're going to comment the writing style of individuals rather than their factual correctness then we'll never agree on anything and have endless conversation going nowhere. I certainly refrained to do so since I'm reviewing other people patches.
> On 31 Mar 2021, at 18:16, Alexandre Janniaux <ajanni at videolabs.io> wrote:
>
> Hi,
>
>> On Wed, Mar 31, 2021 at 05:04:07PM +0200, Steve Lhomme wrote:
>>> On 2021-03-31 13:35, Alexandre Janniaux wrote:
>>> On Wed, Mar 31, 2021 at 10:55:44AM +0200, Steve Lhomme wrote:
>>>> On 2021-03-31 10:47, Alexandre Janniaux wrote:
>>>>> Hi,
>>>>>
>>>>> Sorry for nitpicking, but can a leaner short commit message
>>>>> substitute this one?
>>>>>
>>>>> Maybe:
>>>>>
>>>>> libvlc: select ID3D11RenderTargetView per plane
>>>>
>>>> I can remove the "to use" part to be under 80 chars.
>>>>
>>>
>>> Usual conventions are more 50 characters, but I better
>>> just rephrase into: short commits messages aren´t really
>>> made to describe the whole patchset and intent, but to
>>> provide usable one-liners to identify the changes. The
>>> description commit message is made for such description.
>>>
>>> I´d be glad if you can improve that in any way that suits
>>> you. :)
>>
>> I have always written <= 80 chars in the title and never had a problem until
>> now. I'm not going to follow unwritten rules that no one follows.
>
> I´ll rephrase again in an hopefully clearer way: I don´t care
> about length, but your short commits messages are long, and in
> my opinion for no reason (or well, because they describe the
> content of the commit instead of describing the commit), which
> adds a lot of noise and makes them harder to grasp. If you don´t
> want to fix it then be it, that´s why I mentioned it was
> nitpicking. But that´s far from being the only time it was
> mentioned on the mailing list.
>
> Regards,
> --
> Alexandre Janniaux
> Videolabs
>
>>>>> Regards,
>>>>> --
>>>>> Alexandre Janniaux
>>>>> Videolabs
>>>>>
>>>>> On Wed, Mar 31, 2021 at 08:25:22AM +0200, Steve Lhomme wrote:
>>>>>> This is more flexible as it doesn't depend on a shared ID3D11DeviceContext.
>>>>>>
>>>>>> A NULL callback is still allowed as long as the host app does the
>>>>>> OMSetRenderTargets on their single target.
>>>>>> ---
>>>>>> doc/libvlc/d3d11_player.cpp | 5 ++++-
>>>>>> include/vlc/libvlc_media_player.h | 11 +++++++----
>>>>>> modules/video_output/win32/d3d11_quad.c | 6 +++++-
>>>>>> modules/video_output/win32/d3d11_quad.h | 2 +-
>>>>>> modules/video_output/win32/d3d11_swapchain.c | 6 +++---
>>>>>> modules/video_output/win32/d3d11_swapchain.h | 2 +-
>>>>>> modules/video_output/win32/direct3d11.c | 7 +++++--
>>>>>> 7 files changed, 26 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/doc/libvlc/d3d11_player.cpp b/doc/libvlc/d3d11_player.cpp
>>>>>> index 4cfdce4d51b..7989846bfdd 100644
>>>>>> --- a/doc/libvlc/d3d11_player.cpp
>>>>>> +++ b/doc/libvlc/d3d11_player.cpp
>>>>>> @@ -463,11 +463,14 @@ static bool StartRendering_cb( void *opaque, bool enter )
>>>>>> return true;
>>>>>> }
>>>>>>
>>>>>> -static bool SelectPlane_cb( void *opaque, size_t plane )
>>>>>> +static bool SelectPlane_cb( void *opaque, size_t plane, void *out )
>>>>>> {
>>>>>> + ID3D11RenderTargetView **output = static_cast<ID3D11RenderTargetView**>( out );
>>>>>> struct render_context *ctx = static_cast<struct render_context *>( opaque );
>>>>>> if ( plane != 0 ) // we only support one packed RGBA plane (DXGI_FORMAT_R8G8B8A8_UNORM)
>>>>>> return false;
>>>>>> + // we don't really need to return it as we already do the OMSetRenderTargets().
>>>>>> + *output = ctx->resized.textureRenderTarget;
>>>>>> return true;
>>>>>> }
>>>>>>
>>>>>> diff --git a/include/vlc/libvlc_media_player.h b/include/vlc/libvlc_media_player.h
>>>>>> index 1af9996709d..aaf1e90ffcb 100644
>>>>>> --- a/include/vlc/libvlc_media_player.h
>>>>>> +++ b/include/vlc/libvlc_media_player.h
>>>>>> @@ -710,14 +710,17 @@ typedef void( *libvlc_video_output_set_resize_cb )( void *opaque,
>>>>>> *
>>>>>> * \param opaque private pointer set on the opaque parameter of @a libvlc_video_output_setup_cb() [IN]
>>>>>> * \param plane number of the rendering plane to select
>>>>>> + * \param output handle of the rendering output for the given plane
>>>>>> * \return true on success
>>>>>> * \version LibVLC 4.0.0 or later
>>>>>> *
>>>>>> * \note This is only used with \ref libvlc_video_engine_d3d11.
>>>>>> *
>>>>>> - * The host should call OMSetRenderTargets for Direct3D11. If this callback is
>>>>>> - * not used (set to NULL in @a libvlc_video_set_output_callbacks()) OMSetRenderTargets
>>>>>> - * has to be set during the @a libvlc_video_makeCurrent_cb()
>>>>>> + * The output parameter receives the ID3D11RenderTargetView* to use for rendering
>>>>>> + * the plane.
>>>>>> + *
>>>>>> + * If this callback is not used (set to NULL in @a libvlc_video_set_output_callbacks())
>>>>>> + * OMSetRenderTargets has to be set during the @a libvlc_video_makeCurrent_cb()
>>>>>> * entering call.
>>>>>> *
>>>>>> * The number of planes depend on the DXGI_FORMAT returned during the
>>>>>> @@ -727,7 +730,7 @@ typedef void( *libvlc_video_output_set_resize_cb )( void *opaque,
>>>>>> * This callback is called between libvlc_video_makeCurrent_cb current/not-current
>>>>>> * calls.
>>>>>> */
>>>>>> -typedef bool( *libvlc_video_output_select_plane_cb )( void *opaque, size_t plane );
>>>>>> +typedef bool( *libvlc_video_output_select_plane_cb )( void *opaque, size_t plane, void *output );
>>>>>>
>>>>>> /**
>>>>>> * Set callbacks and data to render decoded video to a custom texture
>>>>>> diff --git a/modules/video_output/win32/d3d11_quad.c b/modules/video_output/win32/d3d11_quad.c
>>>>>> index b8948b16daa..aba095467c1 100644
>>>>>> --- a/modules/video_output/win32/d3d11_quad.c
>>>>>> +++ b/modules/video_output/win32/d3d11_quad.c
>>>>>> @@ -70,9 +70,13 @@ void D3D11_RenderQuad(d3d11_device_t *d3d_dev, d3d11_quad_t *quad, d3d11_vertex_
>>>>>> if (!quad->d3dpixelShader[i])
>>>>>> break;
>>>>>>
>>>>>> - if (unlikely(!selectPlane(selectOpaque, i)))
>>>>>> + ID3D11RenderTargetView *renderView = NULL;
>>>>>> + if (unlikely(!selectPlane(selectOpaque, i, &renderView)))
>>>>>> continue;
>>>>>>
>>>>>> + if (renderView != NULL)
>>>>>> + ID3D11DeviceContext_OMSetRenderTargets(d3d_dev->d3dcontext, 1, &renderView, NULL);
>>>>>> +
>>>>>> ID3D11DeviceContext_PSSetShader(d3d_dev->d3dcontext, quad->d3dpixelShader[i], NULL, 0);
>>>>>>
>>>>>> ID3D11DeviceContext_RSSetViewports(d3d_dev->d3dcontext, 1, &quad->cropViewport[i]);
>>>>>> diff --git a/modules/video_output/win32/d3d11_quad.h b/modules/video_output/win32/d3d11_quad.h
>>>>>> index 01ffca82fd9..07c3a0c9d9a 100644
>>>>>> --- a/modules/video_output/win32/d3d11_quad.h
>>>>>> +++ b/modules/video_output/win32/d3d11_quad.h
>>>>>> @@ -29,7 +29,7 @@
>>>>>> #define PS_CONST_LUMI_BOUNDS 0
>>>>>> #define VS_CONST_VIEWPOINT 1
>>>>>>
>>>>>> -typedef bool (*d3d11_select_plane_t)(void *opaque, size_t plane_index);
>>>>>> +typedef bool (*d3d11_select_plane_t)(void *opaque, size_t plane_index, ID3D11RenderTargetView **);
>>>>>>
>>>>>> void D3D11_RenderQuad(d3d11_device_t *, d3d11_quad_t *, d3d11_vertex_shader_t *,
>>>>>> ID3D11ShaderResourceView *resourceViews[DXGI_MAX_SHADER_VIEW],
>>>>>> diff --git a/modules/video_output/win32/d3d11_swapchain.c b/modules/video_output/win32/d3d11_swapchain.c
>>>>>> index 4b67ec4655d..f48fc0a4670 100644
>>>>>> --- a/modules/video_output/win32/d3d11_swapchain.c
>>>>>> +++ b/modules/video_output/win32/d3d11_swapchain.c
>>>>>> @@ -236,13 +236,13 @@ bool D3D11_LocalSwapchainStartEndRendering( void *opaque, bool enter )
>>>>>> return true;
>>>>>> }
>>>>>>
>>>>>> -bool D3D11_LocalSwapchainSelectPlane( void *opaque, size_t plane )
>>>>>> +bool D3D11_LocalSwapchainSelectPlane( void *opaque, size_t plane, void *out )
>>>>>> {
>>>>>> struct d3d11_local_swapchain *display = opaque;
>>>>>> if (!display->swapchainTargetView[plane])
>>>>>> return false;
>>>>>> - ID3D11DeviceContext_OMSetRenderTargets(display->d3d_dev->d3dcontext, 1,
>>>>>> - &display->swapchainTargetView[plane], NULL);
>>>>>> + ID3D11RenderTargetView **output = out;
>>>>>> + *output = display->swapchainTargetView[plane];
>>>>>> return true;
>>>>>> }
>>>>>>
>>>>>> diff --git a/modules/video_output/win32/d3d11_swapchain.h b/modules/video_output/win32/d3d11_swapchain.h
>>>>>> index f5dd4c4cf47..3d373713405 100644
>>>>>> --- a/modules/video_output/win32/d3d11_swapchain.h
>>>>>> +++ b/modules/video_output/win32/d3d11_swapchain.h
>>>>>> @@ -36,7 +36,7 @@ void *D3D11_CreateLocalSwapchainHandleDComp(vlc_object_t *, void* dcompDevice, v
>>>>>> void D3D11_LocalSwapchainCleanupDevice( void *opaque );
>>>>>> bool D3D11_LocalSwapchainUpdateOutput( void *opaque, const libvlc_video_render_cfg_t *cfg, libvlc_video_output_cfg_t *out );
>>>>>> bool D3D11_LocalSwapchainStartEndRendering( void *opaque, bool enter );
>>>>>> -bool D3D11_LocalSwapchainSelectPlane( void *opaque, size_t plane );
>>>>>> +bool D3D11_LocalSwapchainSelectPlane( void *opaque, size_t plane, void *output );
>>>>>> bool D3D11_LocalSwapchainWinstoreSize( void *opaque, uint32_t *, uint32_t * );
>>>>>> void D3D11_LocalSwapchainSwap( void *opaque );
>>>>>> void D3D11_LocalSwapchainSetMetadata( void *opaque, libvlc_video_metadata_type_t, const void * );
>>>>>> diff --git a/modules/video_output/win32/direct3d11.c b/modules/video_output/win32/direct3d11.c
>>>>>> index 22e4b2a155e..db921eb7321 100644
>>>>>> --- a/modules/video_output/win32/direct3d11.c
>>>>>> +++ b/modules/video_output/win32/direct3d11.c
>>>>>> @@ -471,12 +471,15 @@ static int Control(vout_display_t *vd, int query)
>>>>>> return res;
>>>>>> }
>>>>>>
>>>>>> -static bool SelectRenderPlane(void *opaque, size_t plane)
>>>>>> +static bool SelectRenderPlane(void *opaque, size_t plane, ID3D11RenderTargetView **targetView)
>>>>>> {
>>>>>> vout_display_sys_t *sys = opaque;
>>>>>> if (!sys->selectPlaneCb)
>>>>>> + {
>>>>>> + *targetView = NULL;
>>>>>> return plane == 0; // we only support one packed RGBA plane by default
>>>>>> - return sys->selectPlaneCb(sys->outside_opaque, plane);
>>>>>> + }
>>>>>> + return sys->selectPlaneCb(sys->outside_opaque, plane, (void*)targetView);
>>>>>> }
>>>>>>
>>>>>> static void PreparePicture(vout_display_t *vd, picture_t *picture, subpicture_t *subpicture,
>>>>>> --
>>>>>> 2.29.2
>>>>>>
>>>>>> _______________________________________________
>>>>>> vlc-devel mailing list
>>>>>> To unsubscribe or modify your subscription options:
>>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>> _______________________________________________
>>>>> vlc-devel mailing list
>>>>> To unsubscribe or modify your subscription options:
>>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>>
>>>> _______________________________________________
>>>> vlc-devel mailing list
>>>> To unsubscribe or modify your subscription options:
>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list