search

emersion / xdg-desktop-portal-wlr

xdg-desktop-portal backend for wlroots
MIT License
592 stars 56 forks source link

screencast: don't rely on the structure of xdpw_screencast_instance t… #150

Closed columbarius closed 3 years ago

columbarius commented 3 years ago

…o convert formats

danshick commented 3 years ago

Do we want to change that to SPA_VIDEO_FORMAT_UNKNOWN?

What practical information does that signal to pipewire? I suppose in this case the downstream app can't make sense of it. Probably not a good reason to abruptly crash, but at the same time, it does imply we are seeing a format "in the wild" that we've never seen before. I'd like to encourage the bug reports that abort() brings.

I think what you're proposing is more correct but what we have may be more practical. I seriously doubt we'll ever hit this default case, so I don't mind much either way.

columbarius commented 3 years ago

What practical information does that signal to pipewire?

None, except the Format doesn't have a representation on the pipewire enums.

It's more that i don't like a helper function like format convertion to crash the portal, or other running streams. I would prefer to add the abort either here https://github.com/columbarius/xdg-desktop-portal-wlr/blob/cleanup-format_pw_from_shm/src/screencast/pipewire_screencast.c#L166 or just print a critical error and end the stream instead. An error in the log is probably as usefull as a dump?

danshick commented 3 years ago

Logging is just as useful but less likely to result in a github issue. Still, I think I agree with you, format negotiation is a bad place to crash out, but if wlroots is offering an unknown format, we're already pretty stuck.

columbarius commented 3 years ago

Thanks for merging!