[vlc-devel] [PATCH] display: specify if vout_display_PlacePicture() alignment is top/bottom-left
Alexandre Janniaux
ajanni at videolabs.io
Tue Aug 25 14:45:52 CEST 2020
Hi,
Can this be an enum instead of a bool?
The code duplication in the function is probably not needed
too. Otherwise, LGTM.
Regards,
--
Alexandre Janniaux
Videolabs
On Tue, Aug 25, 2020 at 02:22:12PM +0200, Steve Lhomme wrote:
> For OpenGL the picture alignment must be computed from the bottom-left, whereas
> all other modules align from top-left.
>
> This was handled internally by OpenGL modules, but for center vertical
> alignment on odd vertical dimensions, it may round to the same value regardless
> of the display coordinate system, which is incorrect.
> ---
> include/vlc_vout_display.h | 5 ++-
> modules/hw/mmal/vout.c | 2 +-
> modules/hw/vdpau/display.c | 6 ++--
> modules/video_output/caopengllayer.m | 8 +----
> modules/video_output/ios.m | 2 +-
> modules/video_output/kva.c | 4 +--
> modules/video_output/macosx.m | 14 ++-------
> modules/video_output/opengl/display.c | 18 ++---------
> modules/video_output/vulkan/display.c | 2 +-
> modules/video_output/wayland/shm.c | 4 +--
> modules/video_output/win32/common.c | 6 +++-
> modules/video_output/xcb/render.c | 2 +-
> modules/video_output/xcb/x11.c | 4 +--
> src/video_output/display.c | 45 ++++++++++++++++++++-------
> src/video_output/video_output.c | 2 +-
> 15 files changed, 62 insertions(+), 62 deletions(-)
>
> diff --git a/include/vlc_vout_display.h b/include/vlc_vout_display.h
> index 2a0b593a13e..9afa27d1c78 100644
> --- a/include/vlc_vout_display.h
> +++ b/include/vlc_vout_display.h
> @@ -512,8 +512,11 @@ static inline bool vout_display_PlaceEquals(const vout_display_place_t *p1,
> * \param place Storage space for the picture placement [OUT]
> * \param source Video source format
> * \param cfg Display configuration
> + * \param bottom_left True if the alignement should be done from the bottom-left
> + * otherwise the top-left corner is used.
> */
> -VLC_API void vout_display_PlacePicture(vout_display_place_t *place, const video_format_t *source, const vout_display_cfg_t *cfg);
> +VLC_API void vout_display_PlacePicture(vout_display_place_t *place, const video_format_t *source,
> + const vout_display_cfg_t *cfg, bool bottom_left);
>
> /**
> * Translates mouse state.
> diff --git a/modules/hw/mmal/vout.c b/modules/hw/mmal/vout.c
> index 5889d3d55ec..4fd3b2eb075 100644
> --- a/modules/hw/mmal/vout.c
> +++ b/modules/hw/mmal/vout.c
> @@ -522,7 +522,7 @@ place_dest(vout_display_sys_t * const sys,
> tcfg.display.width = sys->display_width;
> tcfg.display.height = sys->display_height;
> tcfg.is_display_filled = true;
> - vout_display_PlacePicture(&place, fmt, &tcfg);
> + vout_display_PlacePicture(&place, fmt, &tcfg, false);
>
> sys->dest_rect = place_to_mmal_rect(place);
> }
> diff --git a/modules/hw/vdpau/display.c b/modules/hw/vdpau/display.c
> index 5963be63718..4be6bc046bb 100644
> --- a/modules/hw/vdpau/display.c
> +++ b/modules/hw/vdpau/display.c
> @@ -237,7 +237,7 @@ static int Control(vout_display_t *vd, int query, va_list ap)
> vout_display_place_t place;
>
> msg_Dbg(vd, "resetting pictures");
> - vout_display_PlacePicture(&place, src, cfg);
> + vout_display_PlacePicture(&place, src, cfg, false);
>
> fmt->i_width = src->i_width * place.width / src->i_visible_width;
> fmt->i_height = src->i_height * place.height / src->i_visible_height;
> @@ -259,7 +259,7 @@ static int Control(vout_display_t *vd, int query, va_list ap)
> const vout_display_cfg_t *cfg = va_arg(ap, const vout_display_cfg_t *);
> vout_display_place_t place;
>
> - vout_display_PlacePicture(&place, &vd->source, cfg);
> + vout_display_PlacePicture(&place, &vd->source, cfg, false);
> if (place.width != vd->fmt.i_visible_width
> || place.height != vd->fmt.i_visible_height)
> return VLC_EGENERIC;
> @@ -427,7 +427,7 @@ static int Open(vout_display_t *vd, const vout_display_cfg_t *cfg,
> };
> vout_display_place_t place;
>
> - vout_display_PlacePicture(&place, &vd->source, cfg);
> + vout_display_PlacePicture(&place, &vd->source, cfg, false);
> sys->window = xcb_generate_id(sys->conn);
>
> xcb_void_cookie_t c =
> diff --git a/modules/video_output/caopengllayer.m b/modules/video_output/caopengllayer.m
> index 0dafcec000a..14018acd184 100644
> --- a/modules/video_output/caopengllayer.m
> +++ b/modules/video_output/caopengllayer.m
> @@ -319,14 +319,8 @@ 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);
> + vout_display_PlacePicture(&place, &vd->source, &cfg_tmp, true);
> if (unlikely(OpenglLock(sys->gl)))
> // don't return an error or we need to handle VOUT_DISPLAY_RESET_PICTURES
> return VLC_SUCCESS;
> diff --git a/modules/video_output/ios.m b/modules/video_output/ios.m
> index 80b75147c7e..51784b351a8 100644
> --- a/modules/video_output/ios.m
> +++ b/modules/video_output/ios.m
> @@ -645,7 +645,7 @@ static void GLESSwap(vlc_gl_t *gl)
> cfg.display.width = _viewSize.width * _scaleFactor;
> cfg.display.height = _viewSize.height * _scaleFactor;
>
> - vout_display_PlacePicture(place, &_voutDisplay->source, &cfg);
> + vout_display_PlacePicture(place, &_voutDisplay->source, &cfg, false);
> }
>
> - (void)reshape
> diff --git a/modules/video_output/kva.c b/modules/video_output/kva.c
> index 185e8269600..151ee9c1e3c 100644
> --- a/modules/video_output/kva.c
> +++ b/modules/video_output/kva.c
> @@ -433,7 +433,7 @@ static int Control( vout_display_t *vd, int query, va_list args )
> case VOUT_DISPLAY_CHANGE_SOURCE_ASPECT:
> {
> vout_display_place_t place;
> - vout_display_PlacePicture(&place, &vd->source, vd->cfg);
> + vout_display_PlacePicture(&place, &vd->source, vd->cfg, false);
>
> sys->kvas.ulAspectWidth = place.width;
> sys->kvas.ulAspectHeight = place.height;
> @@ -938,7 +938,7 @@ static MRESULT EXPENTRY WndProc( HWND hwnd, ULONG msg, MPARAM mp1, MPARAM mp2 )
> i_movie_height = movie_rect.yTop - movie_rect.yBottom;
>
> vout_display_place_t place;
> - vout_display_PlacePicture(&place, &vd->source, vd->cfg);
> + vout_display_PlacePicture(&place, &vd->source, vd->cfg, false);
>
> int x = ( i_mouse_x - movie_rect.xLeft ) *
> place.width / i_movie_width + place.x;
> diff --git a/modules/video_output/macosx.m b/modules/video_output/macosx.m
> index 15d0c305772..060113b9b14 100644
> --- a/modules/video_output/macosx.m
> +++ b/modules/video_output/macosx.m
> @@ -321,7 +321,7 @@ static void PictureDisplay (vout_display_t *vd, picture_t *pic)
> {
> if (@available(macOS 10.14, *)) {
> vout_display_place_t place;
> - vout_display_PlacePicture(&place, &vd->source, &sys->cfg);
> + vout_display_PlacePicture(&place, &vd->source, &sys->cfg, true);
> vout_display_opengl_Viewport(vd->sys->vgl, place.x,
> sys->cfg.display.height - (place.y + place.height),
> place.width, place.height);
> @@ -355,16 +355,8 @@ static int Control (vout_display_t *vd, int query, va_list ap)
>
> /* we always use our current frame here, because we have some size constraints
> in the ui vout provider */
> - vout_display_cfg_t cfg_tmp = *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, cfg, true);
> @synchronized (sys->glView) {
> sys->cfg = *cfg;
> }
> @@ -627,7 +619,7 @@ static void OpenglSwap (vlc_gl_t *gl)
> sys->cfg.display.width = bounds.size.width;
> sys->cfg.display.height = bounds.size.height;
>
> - vout_display_PlacePicture(&place, &vd->source, &sys->cfg);
> + vout_display_PlacePicture(&place, &vd->source, &sys->cfg, true);
> // FIXME: this call leads to a fatal mutex locking error in vout_ChangeDisplaySize()
> // vout_window_ReportSize(sys->embed, bounds.size.width, bounds.size.height);
> }
> diff --git a/modules/video_output/opengl/display.c b/modules/video_output/opengl/display.c
> index 2db08ccc03b..258a7205c0f 100644
> --- a/modules/video_output/opengl/display.c
> +++ b/modules/video_output/opengl/display.c
> @@ -206,16 +206,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;
> @@ -234,9 +224,7 @@ static int Control (vout_display_t *vd, int query, va_list ap)
> vout_display_cfg_t cfg = *va_arg(ap, const vout_display_cfg_t *);
> const video_format_t *src = &vd->source;
>
> - FlipVerticalAlign(&cfg);
> -
> - vout_display_PlacePicture(&sys->place, src, &cfg);
> + vout_display_PlacePicture(&sys->place, src, &cfg, true);
> sys->place_changed = true;
> vlc_gl_Resize (sys->gl, cfg.display.width, cfg.display.height);
> return VLC_SUCCESS;
> @@ -247,9 +235,7 @@ static int Control (vout_display_t *vd, int query, va_list ap)
> {
> vout_display_cfg_t cfg = *va_arg(ap, const vout_display_cfg_t *);
>
> - FlipVerticalAlign(&cfg);
> -
> - vout_display_PlacePicture(&sys->place, &vd->source, &cfg);
> + vout_display_PlacePicture(&sys->place, &vd->source, &cfg, true);
> sys->place_changed = true;
> return VLC_SUCCESS;
> }
> diff --git a/modules/video_output/vulkan/display.c b/modules/video_output/vulkan/display.c
> index 226165f2838..5fc90e7570a 100644
> --- a/modules/video_output/vulkan/display.c
> +++ b/modules/video_output/vulkan/display.c
> @@ -348,7 +348,7 @@ static int Control(vout_display_t *vd, int query, va_list ap)
> case VOUT_DISPLAY_CHANGE_SOURCE_CROP:
> case VOUT_DISPLAY_CHANGE_ZOOM: {
> vout_display_cfg_t cfg = *va_arg (ap, const vout_display_cfg_t *);
> - vout_display_PlacePicture(&sys->place, &vd->source, &cfg);
> + vout_display_PlacePicture(&sys->place, &vd->source, &cfg, false);
> return VLC_SUCCESS;
> }
>
> diff --git a/modules/video_output/wayland/shm.c b/modules/video_output/wayland/shm.c
> index 77acabdd0c2..5752ab6a351 100644
> --- a/modules/video_output/wayland/shm.c
> +++ b/modules/video_output/wayland/shm.c
> @@ -166,7 +166,7 @@ static int Control(vout_display_t *vd, int query, va_list ap)
> video_format_t src;
> assert(sys->viewport == NULL);
>
> - vout_display_PlacePicture(&place, &vd->source, cfg);
> + vout_display_PlacePicture(&place, &vd->source, cfg, false);
> video_format_ApplyRotation(&src, &vd->source);
>
> fmt->i_width = src.i_width * place.width
> @@ -198,7 +198,7 @@ static int Control(vout_display_t *vd, int query, va_list ap)
> vout_display_place_t place;
>
> video_format_ApplyRotation(&fmt, &vd->source);
> - vout_display_PlacePicture(&place, &vd->source, cfg);
> + vout_display_PlacePicture(&place, &vd->source, cfg, false);
>
> wp_viewport_set_source(sys->viewport,
> wl_fixed_from_int(fmt.i_x_offset),
> diff --git a/modules/video_output/win32/common.c b/modules/video_output/win32/common.c
> index 60fb4e0f25a..f1fc7904c81 100644
> --- a/modules/video_output/win32/common.c
> +++ b/modules/video_output/win32/common.c
> @@ -98,17 +98,21 @@ void CommonPlacePicture(vout_display_t *vd, display_win32_area_t *area, vout_dis
> {
> /* Update the window position and size */
> vout_display_cfg_t place_cfg = area->vdcfg;
> + bool bottom_left;
>
> #if (defined(MODULE_NAME_IS_glwin32))
> + bottom_left = true;
> /* Reverse vertical alignment as the GL tex are Y inverted */
> if (place_cfg.align.vertical == VLC_VIDEO_ALIGN_TOP)
> place_cfg.align.vertical = VLC_VIDEO_ALIGN_BOTTOM;
> else if (place_cfg.align.vertical == VLC_VIDEO_ALIGN_BOTTOM)
> place_cfg.align.vertical = VLC_VIDEO_ALIGN_TOP;
> +#else
> + bottom_left = false;
> #endif
>
> vout_display_place_t before_place = area->place;
> - vout_display_PlacePicture(&area->place, &vd->source, &place_cfg);
> + vout_display_PlacePicture(&area->place, &vd->source, &place_cfg, bottom_left);
>
> /* Signal the change in size/position */
> if (!vout_display_PlaceEquals(&before_place, &area->place))
> diff --git a/modules/video_output/xcb/render.c b/modules/video_output/xcb/render.c
> index c3e71933f35..bb2dc04ced0 100644
> --- a/modules/video_output/xcb/render.c
> +++ b/modules/video_output/xcb/render.c
> @@ -281,7 +281,7 @@ static void CreateBuffers(vout_display_t *vd, const vout_display_cfg_t *cfg)
> sys->format.argb, 0, NULL);
>
> vout_display_place_t *place = &sys->place;
> - vout_display_PlacePicture(place, fmt, cfg);
> + vout_display_PlacePicture(place, fmt, cfg, false);
>
> /* Homogeneous coordinates transform from destination(place)
> * to source(fmt) */
> diff --git a/modules/video_output/xcb/x11.c b/modules/video_output/xcb/x11.c
> index 05e012f3897..65b7f31d897 100644
> --- a/modules/video_output/xcb/x11.c
> +++ b/modules/video_output/xcb/x11.c
> @@ -150,7 +150,7 @@ static int Control(vout_display_t *vd, int query, va_list ap)
> vout_display_place_t place;
> int ret = VLC_SUCCESS;
>
> - vout_display_PlacePicture(&place, &vd->source, cfg);
> + vout_display_PlacePicture(&place, &vd->source, cfg, false);
>
> uint32_t mask = XCB_CONFIG_WINDOW_X | XCB_CONFIG_WINDOW_Y;
> const uint32_t values[] = {
> @@ -313,7 +313,7 @@ static int Open (vout_display_t *vd, const vout_display_cfg_t *cfg,
> };
> vout_display_place_t place;
>
> - vout_display_PlacePicture(&place, &vd->source, cfg);
> + vout_display_PlacePicture(&place, &vd->source, cfg, false);
> sys->window = xcb_generate_id (conn);
> sys->gc = xcb_generate_id (conn);
>
> diff --git a/src/video_output/display.c b/src/video_output/display.c
> index 0112425ac14..688ec3f79b5 100644
> --- a/src/video_output/display.c
> +++ b/src/video_output/display.c
> @@ -143,7 +143,8 @@ void vout_display_GetDefaultDisplaySize(unsigned *width, unsigned *height,
> /* */
> void vout_display_PlacePicture(vout_display_place_t *place,
> const video_format_t *source,
> - const vout_display_cfg_t *cfg)
> + const vout_display_cfg_t *cfg,
> + bool bottom_left)
> {
> /* vout_display_PlacePicture() is called from vd plugins. They should not
> * care about the initial window properties. */
> @@ -210,16 +211,36 @@ void vout_display_PlacePicture(vout_display_place_t *place,
> break;
> }
>
> - switch (cfg->align.vertical) {
> - case VLC_VIDEO_ALIGN_TOP:
> - place->y = 0;
> - break;
> - case VLC_VIDEO_ALIGN_BOTTOM:
> - place->y = cfg->display.height - place->height;
> - break;
> - default:
> - place->y = ((int)cfg->display.height - (int)place->height) / 2;
> - break;
> +
> + if (bottom_left)
> + {
> + switch (cfg->align.vertical) {
> + case VLC_VIDEO_ALIGN_TOP:
> + place->y = cfg->display.height - place->height;
> + break;
> + case VLC_VIDEO_ALIGN_BOTTOM:
> + place->y = 0;
> + break;
> + default:
> + // round to upper value
> + place->y = ((int)cfg->display.height - (int)place->height + 1) / 2;
> + break;
> + }
> + }
> + else
> + {
> + switch (cfg->align.vertical) {
> + case VLC_VIDEO_ALIGN_TOP:
> + place->y = 0;
> + break;
> + case VLC_VIDEO_ALIGN_BOTTOM:
> + place->y = cfg->display.height - place->height;
> + break;
> + default:
> + // round to lower value
> + place->y = ((int)cfg->display.height - (int)place->height) / 2;
> + break;
> + }
> }
> }
>
> @@ -229,7 +250,7 @@ void vout_display_TranslateMouseState(vout_display_t *vd, vlc_mouse_t *video,
> vout_display_place_t place;
>
> /* Translate window coordinates to video coordinates */
> - vout_display_PlacePicture(&place, &vd->source, vd->cfg);
> + vout_display_PlacePicture(&place, &vd->source, vd->cfg, false);
>
> if (place.width <= 0 || place.height <= 0) {
> memset(video, 0, sizeof (*video));
> diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
> index 4f48754cf2e..16da94d7064 100644
> --- a/src/video_output/video_output.c
> +++ b/src/video_output/video_output.c
> @@ -1257,7 +1257,7 @@ static int ThreadDisplayRenderPicture(vout_thread_sys_t *vout, bool is_forced)
> video_format_t fmt_spu;
> if (do_dr_spu) {
> vout_display_place_t place;
> - vout_display_PlacePicture(&place, &vd->source, vd->cfg);
> + vout_display_PlacePicture(&place, &vd->source, vd->cfg, false);
>
> fmt_spu = vd->source;
> if (fmt_spu.i_width * fmt_spu.i_height < place.width * place.height) {
> --
> 2.26.2
>
> _______________________________________________
> 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