After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 703753 - osxvideosink: remove legacy code for passing a windows ID
osxvideosink: remove legacy code for passing a windows ID
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.x
Other Mac OS
: Normal blocker
: 1.3.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 704342 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-07-07 18:22 UTC by Andoni Morales
Modified: 2014-06-28 08:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
osxvideosink: remove legacy code for passwing a windows ID (6.51 KB, patch)
2013-07-07 18:22 UTC, Andoni Morales
none Details | Review
0001-osxvideosink-remove-legacy-code-for-passing-a-window.patch (838 bytes, patch)
2013-07-09 15:02 UTC, Andoni Morales
none Details | Review
0001-osxvideosink-remove-legacy-code-for-passing-a-window.patch (5.90 KB, patch)
2013-07-09 15:03 UTC, Andoni Morales
needs-work Details | Review
0001-osxvideosink-warn-about-the-future-deprecation-of-th.patch (989 bytes, patch)
2013-07-17 11:02 UTC, Andoni Morales
committed Details | Review

Description Andoni Morales 2013-07-07 18:22:58 UTC
Created attachment 248556 [details] [review]
osxvideosink: remove legacy code for passwing a windows ID

"have-ns-view" and the "embed" property was kept in 0.10 for backwards compatibility but it's no longer used in favor of the GstVideoOverlay interface.
Comment 1 Sebastian Dröge (slomo) 2013-07-08 14:21:25 UTC
Unfortunately this was already in a stable 1.0 gst-plugins-good release... I think it's ok to remove now still but others might disagree.
Comment 2 Andoni Morales 2013-07-08 14:27:30 UTC
This sink shouldn't even be in good :)

One can argue it breaks the element's API, but it's also bad API that should never be used.
Comment 3 Tim-Philipp Müller 2013-07-08 14:41:12 UTC
I'm a bit undecided about this. Unless the element didn't work at all anyway, we shouldn't just remove it. On the other hand, the property clearly said 'don't use this'. Perhaps we can as a compromise phase it out more gracefully, e.g. add a g_warning() for now, then remove it completely in 1.3.x ?
Comment 4 Sebastian Dröge (slomo) 2013-07-09 07:13:25 UTC
Fine with me, yes. I'm also fine with just removing it immediately though, nobody should've ever used this in 1.0.

And I agree that this sink should've never been in good ;)
Comment 5 Andoni Morales 2013-07-09 15:02:05 UTC
Created attachment 248737 [details] [review]
0001-osxvideosink-remove-legacy-code-for-passing-a-window.patch

I would remove it completely too. I have attached a new version of the patch with a g_warning in the setter.
Comment 6 Andoni Morales 2013-07-09 15:03:21 UTC
Created attachment 248738 [details] [review]
0001-osxvideosink-remove-legacy-code-for-passing-a-window.patch

Adding the correct file :)
Comment 7 Sebastian Dröge (slomo) 2013-07-09 15:41:45 UTC
Review of attachment 248738 [details] [review]:

::: sys/osxvideo/osxvideosink.m
@@ +457,3 @@
   switch (prop_id) {
     case ARG_EMBED:
+      g_warning ("This property will be deprecated in 1.3");

apps would call this as a result of the message, the message is not sent anymore so... :) Keep the functionality for now and just print a warning.

Also should be 1.4, 1.3 will be just a development release.
Comment 8 Andoni Morales 2013-07-10 15:41:50 UTC
So adding a g_warning to the setter only... but that's like doing nothing because the property's description already warns about not using it :)
Comment 9 Sebastian Dröge (slomo) 2013-07-11 09:21:40 UTC
Not sure, that would add an explicit warning that happens during runtime at least :) Tim?
Comment 10 Tim-Philipp Müller 2013-07-11 17:10:11 UTC
Andoni: true, but the thing is no one checks the description unless they write new code, so people who have working code that they just kept using aren't going to check if the property is still there and what the description says. So I think a run-time warning is in order before removing it.

If I may suggest a wording:

 g_warning ("The \"embed\" property of osxvideosink is deprecated and will be removed in the near future. Use the GstVideoOverlay interface instead.");
Comment 11 Sebastian Dröge (slomo) 2013-07-17 08:17:06 UTC
*** Bug 704342 has been marked as a duplicate of this bug. ***
Comment 12 Sebastian Dröge (slomo) 2013-07-17 08:18:11 UTC
Andoni, any news on this? Can you attach a new patch?
Comment 13 Andoni Morales 2013-07-17 11:02:39 UTC
Created attachment 249394 [details] [review]
0001-osxvideosink-warn-about-the-future-deprecation-of-th.patch
Comment 14 Sebastian Dröge (slomo) 2013-07-17 11:25:26 UTC
Comment on attachment 249394 [details] [review]
0001-osxvideosink-warn-about-the-future-deprecation-of-th.patch

Ok, please commit that. And the original patch after 1.3 is open (so keep this bug open)
Comment 15 Andoni Morales 2013-07-17 11:31:02 UTC
Pushed:

commit fbafca49f818666327bf43265de6b03af873955a
Author: Andoni Morales Alastruey <ylatuya@gmail.com>
Date:   Thu Jul 11 19:45:17 2013 +0200

    osxvideosink: warn about the future deprecation of the "embed" property
Comment 16 Edward Hervey 2013-08-14 06:17:35 UTC
Can we close this now that the warning commit is pushed ? Or should we re-target it to 1.4/blocker to make sure we remove the code when that time comes ?
Comment 17 Sebastian Dröge (slomo) 2013-08-14 08:08:48 UTC
Keep open and remove the code in 1.3.x
Comment 18 Sebastian Dröge (slomo) 2014-06-23 18:41:00 UTC
commit 93653ae5f983d670fa5c24d16da42bbc3ec1e80d
Author: Andoni Morales Alastruey <ylatuya@gmail.com>
Date:   Sun Jul 7 20:18:27 2013 +0200

    osxvideosink: remove legacy code for passing a window handle
    
    "have-ns-view" and the "embed" property was kept in 0.10 for
    backwards compatibility but it's no longer used in favor of
    the GstVideoOverlay interface
    
    https://bugzilla.gnome.org/show_bug.cgi?id=703753