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 628258 - [dshowvideosink] add I420 support
[dshowvideosink] add I420 support
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Windows
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-08-29 17:44 UTC by Thomas Löwe
Modified: 2018-11-03 13:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
I420-Patch (1.90 KB, patch)
2010-08-29 17:45 UTC, Thomas Löwe
none Details | Review
Add support for I420 (4.83 KB, patch)
2010-08-29 19:08 UTC, Andoni Morales
needs-work Details | Review

Description Thomas Löwe 2010-08-29 17:44:20 UTC
Long time ago i've made a patch to support i420 in dshowvideosink.

After the commit from 07/2010 "Improvements contributed from the Moovida projet" this isn't longer working in the evr mode (vmr still works fine).

Could somebody please look at this or implement i420 format for dshow?

Thanks,
Thomas
Comment 1 Thomas Löwe 2010-08-29 17:45:15 UTC
Created attachment 169007 [details] [review]
I420-Patch
Comment 2 Andoni Morales 2010-08-29 19:08:10 UTC
Created attachment 169011 [details] [review]
Add support for I420

Added patch in git format for review.
Comment 3 Andoni Morales 2010-08-29 23:40:33 UTC
Review of attachment 169011 [details] [review]:

::: sys/dshowvideosink/dshowvideofakesrc.cpp
@@ -171,3 +174,3 @@
         srcstride = GST_ROUND_UP_4 ( GST_ROUND_UP_2 (vh->rcSource.right) / 2);
       else {
-        if (component == 1)
+		if (I420) {

In line 160, so should check for YV12 and I420.
Here, check the format using the 'fourcc' variable.

::: sys/dshowvideosink/dshowvideofakesrc.h
@@ +24,3 @@
 #include <gst/gst.h>
+
+bool I420;

The 'subtype' variable should be used here.

::: sys/dshowvideosink/dshowvideosink.cpp
@@ +1433,3 @@
+            "format", GST_TYPE_FOURCC, GST_MAKE_FOURCC ('Y', 'V', '1', '2'), NULL);
+    caps = gst_caps_new_simple ("video/x-raw-yuv", 
+  else if (IsEqualGUID (mediatype->subtype, MEDIASUBTYPE_I420))

The fourcc should be I420 here

@@ +1544,3 @@
         bpp = 16;
+		I420 = 1;
+      case GST_MAKE_FOURCC ('I', '4', '2', '0'):

You can use mediatype->subtype for this
Comment 4 Andoni Morales 2010-08-29 23:40:33 UTC
Review of attachment 169011 [details] [review]:

::: sys/dshowvideosink/dshowvideofakesrc.cpp
@@ -171,3 +174,3 @@
         srcstride = GST_ROUND_UP_4 ( GST_ROUND_UP_2 (vh->rcSource.right) / 2);
       else {
-        if (component == 1)
+		if (I420) {

In line 160, so should check for YV12 and I420.
Here, check the format using the 'fourcc' variable.

::: sys/dshowvideosink/dshowvideofakesrc.h
@@ +24,3 @@
 #include <gst/gst.h>
+
+bool I420;

The 'subtype' variable should be used here.

::: sys/dshowvideosink/dshowvideosink.cpp
@@ +1433,3 @@
+            "format", GST_TYPE_FOURCC, GST_MAKE_FOURCC ('Y', 'V', '1', '2'), NULL);
+    caps = gst_caps_new_simple ("video/x-raw-yuv", 
+  else if (IsEqualGUID (mediatype->subtype, MEDIASUBTYPE_I420))

The fourcc should be I420 here

@@ +1544,3 @@
         bpp = 16;
+		I420 = 1;
+      case GST_MAKE_FOURCC ('I', '4', '2', '0'):

You can use mediatype->subtype for this
Comment 5 Tim-Philipp Müller 2010-09-02 16:34:01 UTC
Seeing that this is a regression it would have been nice if whoever broke it would have put some effort into fixing this in time for the release.

But seeing that we still don't have a good enough patch, I don't really want to wait now and delay the release even further.

Marking as blocker for the next release though.
Comment 6 Andoni Morales 2010-09-02 17:14:08 UTC
This element has never supported I420, so it's not a regression. 
Thomas's patch was never applied upstream because, AFAIK, he didn't open a bug for it, but only attached the patch in the ML
Comment 7 Tim-Philipp Müller 2010-09-02 17:18:04 UTC
> This element has never supported I420, so it's not a regression. 
> Thomas's patch was never applied upstream because, AFAIK, he didn't open a bug
> for it, but only attached the patch in the ML

Ah, I misunderstood, sorry. Let's mark this as enhancement then :)
Comment 8 Thomas Löwe 2010-09-02 17:57:01 UTC
Yes, Andoni is right. I've never made the patch public before because i know it was quick and dirty. Did the same for directdrawsink. ;-)

Would be great to see i420, because yv12 is the same (with swapped planes) and there is no need for an extra colorspace conversion element which produces unnecessary cpu load...
Comment 9 Thomas Löwe 2010-11-13 18:45:26 UTC
Any news about this or really nobody here who can add support for i420 in dshow?
Comment 10 Sebastian Dröge (slomo) 2012-02-07 11:56:16 UTC
Any news on this?
Comment 11 Edward Hervey 2018-11-01 16:27:43 UTC
Automated removal of (bad) usage of the "NONE" target milestone.
Comment 12 GStreamer system administrator 2018-11-03 13:06:18 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/24.