[vlc-devel] [PATCH 14/20] opengl: flip the alignment once on Open
Steve Lhomme
robux4 at ycbcr.xyz
Tue Aug 25 11:48:39 CEST 2020
On 2020-08-25 11:36, Alexandre Janniaux wrote:
> Hi,
>
> On Tue, Aug 25, 2020 at 11:16:13AM +0200, Steve Lhomme wrote:
>> On 2020-08-25 10:39, Alexandre Janniaux wrote:
>>> Hi,
>>>
>>> This seems wrong. Alignment is reversed in OpenGL because
>>> coordinate system is reversed too, so ppicture_place data
>>> need to be reversed too.
>>>
>>> We don't want to report anything to the core and we want
>>> the correct behaviour when doing control. This breaks both
>>> points.
>>>
>>> I'm not sure why you remove the FlipVerticalAlign too.
>>
>> I forgot to add more comments to the commit.
>>
>> Right now the align.vertical is set when enabling the window. After that
>> it's never changed after that (in the core). It's only used inside
>> vout_display_PlacePicture() and nowhere else.
>
> In the future, it could also set by the interface. I don't
> really know why it's not bindable to hotkeys but it makes
> as much sense as other keys like `o` imho.
>
>>
>> Moving this hack to where the actual vout_display_PlacePicture() calls are
>> done (in the core after this patchset) seem logical.
>
>> As explained in the rational for this patchset, the goal is to have the
>> mouse translation using the proper video placement. It's possible that
>> OpenGL is doing things backward.
>
> It's not that OpenGL is doing it backward, it's that OpenGL
> is doing it with a different coordinate system, thus you
> have to adapt what the core request into how OpenGL
> understand it, meaning reverse the y axis.
>
> To be clear, VLC is using usual window coordinates,
> while OpenGL has origin in bottom left corner
>
>
> VLC OpenGL
>
> +----> A
> | |
> | |
> V +------>
>
> Thus, if you don't inverse alignment before placing, the core
> will place the image at the top when it should have been the
> bottom or the opposite.
Given the code uses tricks like this, maybe it's possible to use place.y
and height-place.y in some places
vout_display_opengl_Viewport(sys->vgl, sys->place.x,
bounds.size.height - (sys->place.y +
sys->place.height),
sys->place.width, sys->place.height);
It could be off by one pixel when center aligned vertically.
> Regards,
> --
> Alexandre Janniaux
> Videolabs
>
>>
>>> Regards,
>>> --
>>> Alexandre Janniaux
>>> Videolabs
>>>
>>> On Tue, Aug 25, 2020 at 09:30:02AM +0200, Steve Lhomme wrote:
>>>> ---
>>>> modules/video_output/caopengllayer.m | 12 +++++------
>>>> modules/video_output/macosx.m | 18 +++++++----------
>>>> modules/video_output/opengl/display.c | 29 ++++++++-------------------
>>>> 3 files changed, 21 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/modules/video_output/caopengllayer.m b/modules/video_output/caopengllayer.m
>>>> index e81f8210117..fbdbc8b59dc 100644
>>>> --- a/modules/video_output/caopengllayer.m
>>>> +++ b/modules/video_output/caopengllayer.m
>>>> @@ -213,6 +213,12 @@ static int Open (vout_display_t *vd, vout_display_cfg_t *cfg,
>>>> outputSize = [sys->container visibleRect].size;
>>>> vout_window_ReportSize(sys->embed, (int)outputSize.width, (int)outputSize.height);
>>>>
>>>> + /* Reverse vertical alignment as the GL tex are Y inverted */
>>>> + if (cfg->align.vertical == VLC_VIDEO_ALIGN_TOP)
>>>> + cfg->align.vertical = VLC_VIDEO_ALIGN_BOTTOM;
>>>> + else if (cfg->align.vertical == VLC_VIDEO_ALIGN_BOTTOM)
>>>> + cfg->align.vertical = VLC_VIDEO_ALIGN_TOP;
>>>> +
>>>> return VLC_SUCCESS;
>>>>
>>>> bailout:
>>>> @@ -317,12 +323,6 @@ static int Control (vout_display_t *vd, int query, va_list ap)
>>>> cfg_tmp.display.width = bounds.size.width;
>>>> cfg_tmp.display.height = bounds.size.height;
>>>>
>>>> - /* Reverse vertical alignment as the GL tex are Y inverted */
>>>> - if (cfg_tmp.align.vertical == VLC_VIDEO_ALIGN_TOP)
>>>> - cfg_tmp.align.vertical = VLC_VIDEO_ALIGN_BOTTOM;
>>>> - else if (cfg_tmp.align.vertical == VLC_VIDEO_ALIGN_BOTTOM)
>>>> - cfg_tmp.align.vertical = VLC_VIDEO_ALIGN_TOP;
>>>> -
>>>> vout_display_place_t place;
>>>> vout_display_PlacePicture(&place, &vd->source, &cfg_tmp);
>>>> if (unlikely(OpenglLock(sys->gl)))
>>>> diff --git a/modules/video_output/macosx.m b/modules/video_output/macosx.m
>>>> index 80247f1f29a..ffa6b83ee0f 100644
>>>> --- a/modules/video_output/macosx.m
>>>> +++ b/modules/video_output/macosx.m
>>>> @@ -241,6 +241,12 @@ static int Open (vout_display_t *vd, vout_display_cfg_t *cfg,
>>>> vd->control = Control;
>>>> vd->close = Close;
>>>>
>>>> + /* Reverse vertical alignment as the GL tex are Y inverted */
>>>> + if (cfg->align.vertical == VLC_VIDEO_ALIGN_TOP)
>>>> + cfg->align.vertical = VLC_VIDEO_ALIGN_BOTTOM;
>>>> + else if (cfg->align.vertical == VLC_VIDEO_ALIGN_BOTTOM)
>>>> + cfg->align.vertical = VLC_VIDEO_ALIGN_TOP;
>>>> +
>>>> /* */
>>>> // FIXME: this call leads to a fatal mutex locking error in vout_ChangeDisplaySize()
>>>> // vout_window_ReportSize(sys->embed, fmt->i_visible_width, fmt->i_visible_height);
>>>> @@ -350,18 +356,8 @@ static int Control (vout_display_t *vd, int query, va_list ap)
>>>> case VOUT_DISPLAY_CHANGE_SOURCE_CROP:
>>>> case VOUT_DISPLAY_CHANGE_DISPLAY_SIZE:
>>>> {
>>>> - /* we always use our current frame here, because we have some size constraints
>>>> - in the ui vout provider */
>>>> - vout_display_cfg_t cfg_tmp = *vd->cfg;
>>>> -
>>>> - /* Reverse vertical alignment as the GL tex are Y inverted */
>>>> - if (cfg_tmp.align.vertical == VLC_VIDEO_ALIGN_TOP)
>>>> - cfg_tmp.align.vertical = VLC_VIDEO_ALIGN_BOTTOM;
>>>> - else if (cfg_tmp.align.vertical == VLC_VIDEO_ALIGN_BOTTOM)
>>>> - cfg_tmp.align.vertical = VLC_VIDEO_ALIGN_TOP;
>>>> -
>>>> vout_display_place_t place;
>>>> - vout_display_PlacePicture(&place, &vd->source, &cfg_tmp);
>>>> + vout_display_PlacePicture(&place, &vd->source, vd->cfg);
>>>> @synchronized (sys->glView) {
>>>> sys->cfg = *vd->cfg;
>>>> }
>>>> diff --git a/modules/video_output/opengl/display.c b/modules/video_output/opengl/display.c
>>>> index 2ef8df9910b..a5738310531 100644
>>>> --- a/modules/video_output/opengl/display.c
>>>> +++ b/modules/video_output/opengl/display.c
>>>> @@ -141,6 +141,12 @@ static int Open(vout_display_t *vd, vout_display_cfg_t *cfg,
>>>> if (sys->vgl == NULL)
>>>> goto error;
>>>>
>>>> + /* Reverse vertical alignment as the GL tex are Y inverted */
>>>> + if (cfg->align.vertical == VLC_VIDEO_ALIGN_TOP)
>>>> + cfg->align.vertical = VLC_VIDEO_ALIGN_BOTTOM;
>>>> + else if (cfg->align.vertical == VLC_VIDEO_ALIGN_BOTTOM)
>>>> + cfg->align.vertical = VLC_VIDEO_ALIGN_TOP;
>>>> +
>>>> vd->sys = sys;
>>>> vd->info.subpicture_chromas = spu_chromas;
>>>> vd->prepare = PictureRender;
>>>> @@ -206,16 +212,6 @@ static void PictureDisplay (vout_display_t *vd, picture_t *pic)
>>>> }
>>>> }
>>>>
>>>> -static void
>>>> -FlipVerticalAlign(vout_display_cfg_t *cfg)
>>>> -{
>>>> - /* Reverse vertical alignment as the GL tex are Y inverted */
>>>> - if (cfg->align.vertical == VLC_VIDEO_ALIGN_TOP)
>>>> - cfg->align.vertical = VLC_VIDEO_ALIGN_BOTTOM;
>>>> - else if (cfg->align.vertical == VLC_VIDEO_ALIGN_BOTTOM)
>>>> - cfg->align.vertical = VLC_VIDEO_ALIGN_TOP;
>>>> -}
>>>> -
>>>> static int Control (vout_display_t *vd, int query, va_list ap)
>>>> {
>>>> vout_display_sys_t *sys = vd->sys;
>>>> @@ -231,12 +227,7 @@ static int Control (vout_display_t *vd, int query, va_list ap)
>>>> case VOUT_DISPLAY_CHANGE_DISPLAY_FILLED:
>>>> case VOUT_DISPLAY_CHANGE_ZOOM:
>>>> {
>>>> - vout_display_cfg_t cfg = *vd->cfg;
>>>> - const video_format_t *src = &vd->source;
>>>> -
>>>> - FlipVerticalAlign(&cfg);
>>>> -
>>>> - vout_display_PlacePicture(&sys->place, src, &cfg);
>>>> + vout_display_PlacePicture(&sys->place, &vd->source, vd->cfg);
>>>> sys->place_changed = true;
>>>> vlc_gl_Resize (sys->gl, vd->cfg->display.width, vd->cfg->display.height);
>>>> return VLC_SUCCESS;
>>>> @@ -245,11 +236,7 @@ static int Control (vout_display_t *vd, int query, va_list ap)
>>>> case VOUT_DISPLAY_CHANGE_SOURCE_ASPECT:
>>>> case VOUT_DISPLAY_CHANGE_SOURCE_CROP:
>>>> {
>>>> - vout_display_cfg_t cfg = *vd->cfg;
>>>> -
>>>> - FlipVerticalAlign(&cfg);
>>>> -
>>>> - vout_display_PlacePicture(&sys->place, &vd->source, &cfg);
>>>> + vout_display_PlacePicture(&sys->place, &vd->source, vd->cfg);
>>>> sys->place_changed = true;
>>>> return VLC_SUCCESS;
>>>> }
>>>> --
>>>> 2.26.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
>
More information about the vlc-devel
mailing list