Skip to content

vo_xv, vo_x11: make use of XShm 1.2 #16153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

brad0
Copy link
Contributor

@brad0 brad0 commented Mar 30, 2025

No description provided.

@brad0 brad0 marked this pull request as draft March 30, 2025 11:41
dependency('xrandr', version: '>= 1.4.0', required: x11_opt),
dependency('x11-xcb', version: '>= 1.8.9', required: x11_opt),
dependency('xcb-shm', version: '>= 1.16', required: x11_opt),
dependency('xcb', version: '>= 1.16', required: x11_opt),]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't available on Debian Stable, and considering the whole purpose of this VO is for compatibility with old systems, I'm not sure we want to require this to build X11 at all. Can this be an optional dep?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XCB only seems to be used for a single call that could probably be done without XCB.

p->myximage[foo]->height,
IPC_CREAT | 0777);
len = p->myximage[foo]->bytes_per_line * p->myximage[foo]->height;
memcpy(p->shmname, "/tmp/mpv-x11-XXXXXXXXXX", sizeof(p->shmname));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this rely on TMPDIR?

Copy link
Member

@kasper93 kasper93 Mar 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be memfd_create and on platforms that doesn't support it, it should be made as a platform specific impl in osdep.

p->Shminfo[foo].shmaddr = (char *) shmat(p->Shminfo[foo].shmid, 0, 0);
p->Shminfo[foo].shmaddr = mmap(NULL, len,
PROT_READ | PROT_WRITE,
MAP_SHARED | __MAP_NOFAULT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flag seems to be openbsd-specific

IPC_CREAT | 0777);
len = p->myximage[foo]->bytes_per_line * p->myximage[foo]->height;
memcpy(p->shmname, "/tmp/mpv-x11-XXXXXXXXXX", sizeof(p->shmname));
p->Shminfo[foo].shmid = shm_mkstemp(p->shmname);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another function that does not exist on Linux

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was not aware of this being an extension function until after it went through CI.

PROT_READ | PROT_WRITE,
MAP_SHARED | __MAP_NOFAULT,
p->Shminfo[foo].shmid, 0);
ftruncate(p->Shminfo[foo].shmid, len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this happen before you map it?

shminfo->shmseg = xcb_generate_id(xcb_conn);
xcb_shm_attach_fd(xcb_conn, shminfo->shmseg, shminfo->shmid,
shminfo->readOnly);
return 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointless return value

IPC_CREAT | 0777);
ctx->Shminfo[foo].shmaddr = shmat(ctx->Shminfo[foo].shmid, 0, 0);
memcpy(ctx->shmname, "/tmp/mpv-xv-XXXXXXXXXX",
sizeof(ctx->shmname));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an OOB-read because the string is surely smaller than the destination buffer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most secure OpenBSD code

dependency('xrandr', version: '>= 1.4.0', required: x11_opt),
dependency('x11-xcb', version: '>= 1.8.9', required: x11_opt),
dependency('xcb-shm', version: '>= 1.16', required: x11_opt),
dependency('xcb', version: '>= 1.16', required: x11_opt),]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XCB only seems to be used for a single call that could probably be done without XCB.

@N-R-K
Copy link
Contributor

N-R-K commented Mar 30, 2025

What's the motivation for this anyways? The commit message doesn't really mention anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants