make the osd as large as the screen for vo=drm #5445

sgerwk · 2018-01-24T13:31:14Z

I agree that my changes can be relicensed to LGPL 2.1 or later.

ghost · 2018-01-27T03:58:58Z

video/out/vo_drm.c

@@ -320,21 +314,33 @@ static void draw_image(struct vo *vo, mp_image_t *mpi)
            src_rc.x0 = MP_ALIGN_DOWN(src_rc.x0, mpi->fmt.align_x);
            src_rc.y0 = MP_ALIGN_DOWN(src_rc.y0, mpi->fmt.align_y);
            mp_image_crop_rc(&src, src_rc);
+
+            w = p->dst.x1 - p->dst.x0;
+	    h = p->dst.y1 - p->dst.y0;


Don't use tabs for indentation.

ghost · 2018-01-27T03:59:54Z

CC @rr- (I think?)

xantoz · 2018-02-04T02:53:10Z

video/out/vo_drm.c

+
+            shift = x * BYTES_PER_PIXEL + y * p->cur_frame->stride[0];
+            temp = p->cur_frame->planes[0];
+            p->cur_frame->planes[0] += shift;


This feels a bit hacky IMO. Is there any better way, not involving modifying this pointer?

Setting up an avfilter with pad=..., maybe. But seems that no other vo does anything like this; I don't know if it's really doable this way.

The original vo_drm.c does shifting in the same way, by adding an offset to the pointer to an image being copied. The difference is that then p->cur_frame->planes[0] was the source of the copy, in my version is the target.

Wait, maybe I misunderstood your comment. Did you mean something like creating a new image with image->planes[0] equal to p->cur_frame->planes[0] + shift?

Yes. I had something like that in mind (the latter). At least as long as there is no "proper" way to have mp_sws_scale write at an offset.

xantoz · 2018-02-04T02:53:56Z

video/out/vo_drm.c

+
+            w = p->dst.x1 - p->dst.x0;
+            h = p->dst.y1 - p->dst.y0;
+            x = (p->cur_frame->w - w) >> 1;


Perhaps better to write (p->cur_frame->w - w)/2 here for clarity (and consider having some of these variables be unsigned?). Or is it a requirement to always round down towards negative infinity vs. rounding towards 0?

(This is is just a nitpick though)

I tried to follow the same coding style of the original file, which had these variables signed and >>1 instead of /2. I agree that at least w and h should be unsigned, and dividing by two is clearer than shifting.

sgerwk · 2018-02-07T11:03:03Z

I created a dummy reference to the cur_frame image and then cropped it. This way, swscale on the dummy reference automatically places the scaled copy in the right position. The code is also simpler this way.

Perhaps it also makes sense to move the four clear_area() calls to mp_image.c, something like mp_image_clear_outside() and mp_image_clear_outside_rc().

xantoz · 2018-02-10T07:45:29Z

seems like 8c9e9f3 fixes #5438

xantoz · 2018-02-10T07:49:54Z

Could you squash the first three commits together, since they're all the ones about the OSD problem.
Then reword the commit messages for all commits such that they begin: "vo_drm: <subject here>", and perhaps write a longer description in the body of each.
You should write something in the body of the commit message of 8c9e9f3 about what issue it solves.
Also your name is different between some of the commits, so make sure to fix that too.

Otherwise, LGTM

sgerwk · 2018-02-10T09:48:16Z

Done. Thanks!

kevmitch · 2018-02-11T01:35:40Z

All three commits appear to work as advertised. Importantly, the first one doesn't seem to have significant performance impact playing a 4:3 1080p on a 16:9 1080p display.

Could you put those messages in the commits themselves (wrapped to 72 columns)?

As per contribute.md, the commit message should talk about the behaviour before the commit in past tense and use present tense for the new state of affairs.

which can then be added the osd

"which can then have the osd added to it"

The last image is stored in vo->priv->last_input to be used when redrawing a freme is necessary (contol: VOCTRL_REDRAW_FRAME).

frame, control

For example, if drm mode 5 is "640x400" the right aspect on a 4:3 monitor is obtained by mpv

comma after "640x400"

so the monitorpixel aspect must be set manually? I only have a single 1:1 drm mode here so can't test.

sgerwk · 2018-02-11T10:00:25Z

Fixed comments, moved them to the commit messages.

Yes, you currently have to set --monitorpixelaspect manually, otherwise 1:1 pixels are assumed.

Actually, the drm modes are suggestions, not the only resolutions that can be used. In X11, you can see the drm modes by xrandr, but then add new ones like (assuming the connector you use is LVDS):

cvt 640 400
xrandr --newmode (output of cvt, with the word "Modeline" removed)
xrandr --addmode LVDS (mode name: first string in cvt output after "Modeline")
xrandr --output LVDS --mode (mode name)

In the linux vt, the only way I found to have a new drm mode is to add it to the kernel parameters at startup, like "video=640x400R-16". Actually, --drm-mode=n is a limitation, since it constraints mpv to run only in one of the modes that are present at startup.

kevmitch · 2018-02-11T10:33:29Z

Thanks. That's much better. Just a couple more tweaks

the drm vo drawn the osd over the scaled image

the drm vo drew

This commit deallocates and reset to NULL

deallocates and resets

Before this commit, the drm vo drew the osd over the scaled image, and then copied the result onto the framebuffer, shifted. This made the frame centered, but forced the osd to be only as large as the image. This was inconsistent with other vo's, covered the image with the progress indicator even when a black band was at the top of the screen, made the progress indicator wrap on narrow videos, etc. The change is to always use an image as large as the screen. The frame is copied scaled and shifted to it, and the osd drawn over it. The result is finally copied to the framebuffer without any shift, since it is already as large as it. Technically, cur_frame is an image as large as the screen and cur_frame_cropped is a dummy reference to it, cropped to the size of the scaled video. This way, copying the scaled image to cur_frame_cropped positions the image in the right place in cur_frame, which can then have the osd added to it and copied to the framebuffer.

The last image is stored in vo->priv->last_input to be used when redrawing a frame is necessary (control: VOCTRL_REDRAW_FRAME). At the beginning it is NULL, so a redraw request has no effect since draw_image ignores calls with image=NULL. When using --force-window the size of the image may change without the vo structure being re-created. Before this commit, the size of vo->priv->last_input could become inconsistent with the cropping rectangle vo->priv->src_rc, which could trigger an assert in mp_image_crop_rc(). Even if it did not, the last image of a video remained on the screen when the next file in the playlist had no video (e.g., it was an mp3 without an embedded cover). This commit deallocates and resets to NULL the image vo->priv->last_input when reconfiguring video.

This commit allows for video to be shown with the right aspect even when pixels are not square in the selected drm mode. For example, if drm mode 5 is "640x400", the right aspect on a 4:3 monitor is obtained by mpv --vo=drm --drm-mode=5 --monitorpixelaspect=5:6 ... Other vo's seem to make this parameter change the size of the window, but in the drm vo this is fixed, being as large as the screen.

kevmitch · 2018-02-12T01:51:27Z

thanks.

ghost reviewed Jan 27, 2018

View reviewed changes

xantoz reviewed Feb 4, 2018

View reviewed changes

sgerwk added 3 commits February 11, 2018 12:03

kevmitch merged commit 7916201 into mpv-player:master Feb 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make the osd as large as the screen for vo=drm #5445

make the osd as large as the screen for vo=drm #5445

sgerwk commented Jan 24, 2018

ghost Jan 27, 2018

ghost commented Jan 27, 2018

xantoz Feb 4, 2018

sgerwk Feb 4, 2018

sgerwk Feb 4, 2018

xantoz Feb 6, 2018 •

edited

Loading

xantoz Feb 4, 2018 •

edited

Loading

sgerwk Feb 4, 2018

sgerwk commented Feb 7, 2018 •

edited

Loading

xantoz commented Feb 10, 2018

xantoz commented Feb 10, 2018 •

edited

Loading

sgerwk commented Feb 10, 2018

kevmitch commented Feb 11, 2018

sgerwk commented Feb 11, 2018

kevmitch commented Feb 11, 2018

kevmitch commented Feb 12, 2018

make the osd as large as the screen for vo=drm #5445

make the osd as large as the screen for vo=drm #5445

Conversation

sgerwk commented Jan 24, 2018

ghost Jan 27, 2018

Choose a reason for hiding this comment

ghost commented Jan 27, 2018

xantoz Feb 4, 2018

Choose a reason for hiding this comment

sgerwk Feb 4, 2018

Choose a reason for hiding this comment

sgerwk Feb 4, 2018

Choose a reason for hiding this comment

xantoz Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

xantoz Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

sgerwk Feb 4, 2018

Choose a reason for hiding this comment

sgerwk commented Feb 7, 2018 • edited Loading

xantoz commented Feb 10, 2018

xantoz commented Feb 10, 2018 • edited Loading

sgerwk commented Feb 10, 2018

kevmitch commented Feb 11, 2018

sgerwk commented Feb 11, 2018

kevmitch commented Feb 11, 2018

kevmitch commented Feb 12, 2018

xantoz Feb 6, 2018 •

edited

Loading

xantoz Feb 4, 2018 •

edited

Loading

sgerwk commented Feb 7, 2018 •

edited

Loading

xantoz commented Feb 10, 2018 •

edited

Loading