Avoid using subtitles of a previously played media
ClosedPublic

Authored by meven on Apr 8 2019, 1:59 PM.

Details

Reviewers
dvratil
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
R488:73128a60dae2: Avoid using subtitles of a previously played media
Summary

When a media object detected a subtitle, this one is used in all subsequent playback until another subtitle is set.

Test Plan

Localy tested.

Diff Detail

Repository
R488 Phonon: GStreamer backend
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Apr 8 2019, 1:59 PM
Owners added a reviewer: Restricted Owners Package.Apr 8 2019, 1:59 PM
meven requested review of this revision.Apr 8 2019, 1:59 PM
meven added inline comments.Apr 8 2019, 2:04 PM
gstreamer/mediaobject.cpp
342

This cleanup could be done conditionally or outside this if block.

meven marked an inline comment as done.Apr 8 2019, 2:04 PM
meven updated this revision to Diff 55751.Apr 8 2019, 3:38 PM

Clean previous subtitle unconditionnaly

dvratil requested changes to this revision.Apr 9 2019, 9:43 AM
dvratil added inline comments.
gstreamer/mediaobject.cpp
328

I think it would be better to call changeSubUri(Mrl()); from here, and adjust changeSubUri() to set suburi to NULL when mrl.isNull() and to mrl.toEncoded().costData() otherwise.

This revision now requires changes to proceed.Apr 9 2019, 9:43 AM
meven updated this revision to Diff 55805.Apr 9 2019, 9:53 AM

Use changeSubUri to clean the subtitle

meven marked an inline comment as done.Apr 9 2019, 9:57 AM
meven added inline comments.
gstreamer/mediaobject.cpp
328

I sort of did what you suggested, but I'd rather avoid calling qgetenv for all media files only those that do have a subtitle since this cleaning will always be called.

meven marked 2 inline comments as done.Apr 9 2019, 9:57 AM
dvratil requested changes to this revision.Apr 9 2019, 12:25 PM
dvratil added inline comments.
gstreamer/mediaobject.cpp
306

Shouldn't the be one more NULL argument to terminate the list of properties? Sorry I haven't noticed that before, my GLib-fu is getting weaker ...

328

Good call!

This revision now requires changes to proceed.Apr 9 2019, 12:25 PM
meven marked 2 inline comments as done.Apr 9 2019, 5:01 PM
meven added inline comments.
gstreamer/mediaobject.cpp
306

Well, I did this on purpose : I thought it does not matter not to remove the previously set subtitle font related properties, as without the suburi they are useless.
In the end it is cpu usage against memory, I opted for cpu, since this code block will get called very often.

I don't know well glib myself, I am not sure this is the best way to, but I couldn't find any other way and it works.

meven marked 2 inline comments as done.Apr 11 2019, 7:58 AM
meven added inline comments.Apr 11 2019, 11:18 AM
gstreamer/mediaobject.cpp
306

@dvratil at least it works. It could not find any online documentation about removing/unsetting a g_object property.

meven marked an inline comment as done.Apr 11 2019, 11:19 AM
meven updated this revision to Diff 55980.Apr 11 2019, 11:25 AM

Use g_object_set_property to unset the suburi property

dvratil accepted this revision.Apr 12 2019, 8:20 AM

Looks good now, thanks!

gstreamer/mediaobject.cpp
306

g_object_set can take multiple property-value argument pairs, but the very last argument must be NULL so that the code knows there are no more arguments after it, which is why I asked for the NULL. Anyway, using g_object_set_property is also a good alternative, since we are only changing a single property :)

This revision is now accepted and ready to land.Apr 12 2019, 8:20 AM
meven marked an inline comment as done.Apr 12 2019, 8:50 AM
This revision was automatically updated to reflect the committed changes.