-
Notifications
You must be signed in to change notification settings - Fork 56
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
Labels
enhancement
A feature or change request for the library
Comments
KitsuneRal
changed the title
(Temporarily?) sunset Qt Multimedia usage
Sunset Qt Multimedia usage
Jul 16, 2021
Make sense also QMediaResource::resolution didn't really work well for in my experience |
Yes, that too. |
9a5fa62 broke the build with Qt 6; stay tuned. |
...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
Although Qt 6.2 finally has got Qt Multimedia, libQuotient
cannotnever could make proper use of it. To hold thedependency 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 aQFileInfo
(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 appropriateEventContent
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 thatRoomMessageEvent
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 tooriginalFilename
, 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 (withQMediaPlayer
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 thatQMediaPlayer
instance and waiting on availability of the metadata before uploading the file and constructing a message event for it.The text was updated successfully, but these errors were encountered: