GNOME Bugzilla – Bug 793865
msdk: encode to wrong color with I420 input.
Last modified: 2018-03-29 05:48:38 UTC
$ gst-launch-1.0 videotestsrc ! video/x-raw,format=I420,width=1920,height=1080 ! msdkh264enc ! msdkh264dec ! videoconvert ! ximagesink Then the display show with wrong color picture. Same issue with all msdk encode.
It seems msdk only supporting YV12. For I420, we should swap the Y and V pointers something like we did in gstreamer-vaapi. It is better to disable the I420 support (unless someone come up with a quick patch to do the swapping) for 1.14 release.
Created attachment 369395 [details] [review] disable I420 support.
Created attachment 369426 [details] [review] Enable I420 support.
@Sree, I have cooked the I420 support patch. So please my first disable I420 patch.
(In reply to Fei from comment #4) > @Sree, I have cooked the I420 support patch. So please my first disable I420 > patch. Typo: So please ignore my first disable I420 patch.
Review of attachment 369426 [details] [review]: Please rebase.
Created attachment 369477 [details] [review] Enable I420 support. @Sree, rebased.
Review of attachment 369477 [details] [review]: ::: sys/msdk/gstmsdkvideomemory.c @@ +173,3 @@ + /* For msdk doesn't support I420 format, we need to swap U/V plane's pointer. */ + if (meta->format == GST_VIDEO_FORMAT_I420 && plane ==0) { Wrong, it is completely legit to invoke this method only for plane 1 & 2 only. For eg: the downstream element trying to get only the U component for checksum analysis. @@ +177,3 @@ + SWAP_UINT (mem_id->image.pitches[1], mem_id->image.pitches[2]); +} + This changes the actual image offset which is not a good way to handle it. Because the underlined VAImage still keeping YV12 format. Ideally we should do something similar to gstremaer-vaai by keeping an alternate image. But for now, you may just return the swapped offset and pitch without altering the underlined VAImage structure.
Created attachment 369560 [details] [review] Enable I420 support. @Sree, Modified, please have a review. Thanks.
Review of attachment 369560 [details] [review]: ::: sys/msdk/gstmsdkvideomemory.c @@ +177,3 @@ + SWAP_UINT (mem_id->image.offsets[1], mem_id->image.offsets[2]); + SWAP_UINT (mem_id->image.pitches[1], mem_id->image.pitches[2]); + } Please don't alter the actual VAImage Structure. Check my previous comment. You can just return the changed offset and pitch for I420 so that the underlined VAImage won't be dirty.
Created attachment 369599 [details] [review] Enable I420 support. @Sree, Modified.
Review of attachment 369395 [details] [review]: rejected
Review of attachment 369599 [details] [review]: I will push a slightly different patch.
commit 0c69867d523a560c6ca55afdb2880a62abcab632 Author: Wang,Fei <fei.w.wang@intel.com> Date: Tue Mar 13 13:54:17 2018 -0800 msdk: Fix the I420 video format support Make sure I420 surface mapping works as expected by using YV12 format and swap U/V plane's offset and pitches. https://bugzilla.gnome.org/show_bug.cgi?id=793865
This issue can not be duplicated with commit 48cde372d6f5b9feff464bffb734686e0cbf0ca1, labeled release 1.14.0, so close it.