Skip to content
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

Revise Qt Multimedia usage #483

Open
KitsuneRal opened this issue Jul 16, 2021 · 4 comments
Open

Revise Qt Multimedia usage #483

KitsuneRal opened this issue Jul 16, 2021 · 4 comments
Assignees
Labels
enhancement A feature or change request for the library

Comments

@KitsuneRal
Copy link
Member

KitsuneRal commented Jul 16, 2021

Although Qt 6.2 finally has got Qt Multimedia, libQuotient cannot never could make proper use of it. To hold the
dependency just for the sake of extracting metadata from an audio/video file in one particular place looks like a waste; and without a GUI it is rather challenging to provide any added value on top of that. Moreover, the RoomMessageEvent constructor accepting a QFileInfo (the only part of Quotient actually using Qt Multimedia, and even that uses literally one method, QMediaResource::resolution, for the video image size) is not extensible: it doesn't support adding new kinds of content which will become a shortcoming once extensible events land in Matrix.

Therefore I think of deprecating this constructor if compiled with Qt 5, and entirely dropping it if compiled with Qt 6 (building with Qt 6 is an experimental configuration anyway for now; and its Qt Multimedia API doesn't allow to read even that image size value synchronously anymore). Clients will have to create an appropriate EventContent object themselves. It's likely that they already have everything necessary to extract content metadata if they provide any sort of audio/video/image preview; and even if not, it boils down to making a round over an event loop to read the metadata from the media file.

This would also lower the Qt 6 requirement to from 6.2 to 6.0, although versions before 6.2 are immature and aren't expected to be used much anyway.

Update: after moving around the code I remembered why I introduced both Room::postFile() and that RoomMessageEvent constructor in the first place: the current API to construct event content objects is quite low-level and desperately calls for something simpler on top of it. For example, it doesn't protect from a very easy mistake to pass the full path instead of just a file name to originalFilename, leading to disclosure of the local file structure to Matrix. While it may be worth it to retain the low-level API close to what it is now (with QMediaPlayer as an additional source of metadata when they are immediately available), Room could provide asynchronous means to load media metadata from a file by creating that QMediaPlayer instance and waiting on availability of the metadata before uploading the file and constructing a message event for it.

@KitsuneRal KitsuneRal changed the title (Temporarily?) sunset Qt Multimedia usage Sunset Qt Multimedia usage Jul 16, 2021
@KitsuneRal KitsuneRal self-assigned this Jul 16, 2021
@KitsuneRal KitsuneRal added the enhancement A feature or change request for the library label Jul 16, 2021
@CarlSchwan
Copy link
Contributor

Make sense also QMediaResource::resolution didn't really work well for in my experience

@KitsuneRal
Copy link
Member Author

Yes, that too.

@KitsuneRal
Copy link
Member Author

KitsuneRal commented Jul 17, 2021

9a5fa62 broke the build with Qt 6; stay tuned.

@KitsuneRal KitsuneRal reopened this Jul 17, 2021
@KitsuneRal KitsuneRal changed the title Sunset Qt Multimedia usage Revise Qt Multimedia usage Jul 18, 2021
@KitsuneRal
Copy link
Member Author

...and, while fixing the build breakage, I recollected the reason for the current code. I think Qt Multimedia is here to stay; Quotient just has to make better use of it.

KitsuneRal added a commit that referenced this issue Jul 18, 2021
9a5fa62 dropped one of RoomMessageEvent constructors for Qt 6 in order
to address #483 - breaking the build with Qt 6 along the way, as
Room::postFile() relied on that constructor. This commit changes
Room::postFile() in turn, deprecating the current signature and adding
a new one that accepts an EventContent object rather than a path to
a file. In order to achieve that, FileInfo and ImageInfo classes have
gained new constructors that accept QFileInfo instead of the legacy
series of parameters, streamlining usage of EventContent structures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or change request for the library
Projects
Status: 0.10(?) - To Do
Development

No branches or pull requests

2 participants