GNOME Bugzilla – Bug 789847
msdkenc: Add support for YV12, YUY2, UYVY and BGRA
Last modified: 2017-12-07 18:47:51 UTC
This does not work yet, I'm only putting it into Bugzilla for discussion now
Created attachment 362882 [details] [review] msdkenc: Add support for YV12, YUY2, UYVY and BGRA By doing conversion with VPP to NV12 before the actual encoding.
Created attachment 363035 [details] [review] msdkenc: Add support for YV12, YUY2, UYVY and BGRA By doing conversion with VPP to NV12 before the actual encoding.
Review of attachment 363035 [details] [review]: Still does not work, but some notes for later ::: sys/msdk/gstmsdkenc.c @@ +393,3 @@ + request[0].NumFrameSuggested = + MAX (request[0].NumFrameSuggested, request[1].NumFrameSuggested); + if (request[0].NumFrameSuggested < thiz->param.AsyncDepth) { This should probably be 1 more if vpp @@ +881,3 @@ + + /* Wait for VPP operation to complete */ + MFXVideoCORE_SyncOperation (session, vpp_sync_point, 10000); Instead of waiting for completion here, according to the encoding example from the MediaSDK code, it would also be possible to directly encode it and the encoder takes care of the waiting once needed.
Created attachment 363036 [details] [review] msdkenc: Add support for YV12, YUY2, UYVY and BGRA By doing conversion with VPP to NV12 before the actual encoding.
Review of attachment 363036 [details] [review]: ::: sys/msdk/gstmsdkenc.c @@ +297,3 @@ + thiz->vpp_surfaces = g_new0 (mfxFrameSurface1, thiz->num_vpp_surfaces); + for (i = 0; i < thiz->num_vpp_surfaces; i++) { + thiz->vpp_param.vpp.In.FourCC = MFX_FOURCC_RGB4; This should be thiz->vpp_param.vpp instead of param.vpp.
(In reply to Sebastian Dröge (slomo) from comment #3) > @@ +881,3 @@ > + > + /* Wait for VPP operation to complete */ > + MFXVideoCORE_SyncOperation (session, vpp_sync_point, 10000); > > Instead of waiting for completion here, according to the encoding example > from the MediaSDK code, it would also be possible to directly encode it and > the encoder takes care of the waiting once needed. Yeah, as far as I tested, it seems to work without the operation.
With this patch(modified by comment 5) and the patch in #bug 790312, it works except for YUY2, BGRA. I'm looking into it.
(In reply to Hyunjun Ko from comment #7) > With this patch(modified by comment 5) and the patch in #bug 790312, it > works except for YUY2, BGRA. > > I'm looking into it. YUY2 is only for jpeg encoder as per sample, but not working with msdkmjpegenc. eg. gst-launch-1.0 videotestsrc num-buffers=100 ! video/x-raw,format=YUY2,width=640,height=320 ! msdkmjpegenc ! filesink location=test.jpeg
Review of attachment 363036 [details] [review]: ::: sys/msdk/msdk.c @@ +92,3 @@ + surface->Data.Y = GST_VIDEO_FRAME_COMP_DATA (frame, 0); + surface->Data.U = GST_VIDEO_FRAME_COMP_DATA (frame, 1); + break; Probably this should be 2?
Created attachment 363814 [details] [review] msdkenc: Add support for YV12, YUY2, UYVY and BGRA By doing conversion with VPP to NV12 before the actual encoding.
Created attachment 363815 [details] [review] msdkenc: Add support for YV12, YUY2, UYVY and BGRA By doing conversion with VPP to NV12 before the actual encoding.
Thanks for the review Hyunjun, and for finding my silly mistakes :) YUY2 should work for other codecs too AFAIU, from where do you have the information that it only works for JPEG?
Also the conversion is done by VPP, so first converting YUY2 to NV12 and then passing it to the h264 encoder for example should work fine, or not? Or do you mean YUY2 is supposed to be *natively* handled by the JPEG encoder but not by others?
(In reply to Sebastian Dröge (slomo) from comment #13) > Also the conversion is done by VPP, so first converting YUY2 to NV12 and > then passing it to the h264 encoder for example should work fine, or not? > > Or do you mean YUY2 is supposed to be *natively* handled by the JPEG encoder > but not by others? No, I meant YUY2 -> NV12 -> encoding seems to be only for jpeg encoder. This was according to the sample_encoder's help, which says "YUY2 is for JPEG encode only." But I realized I'm wrong. I modified the sample encoder and verified it(yuy2) also works for h264encoder. So please forget my previous comment about yuy2 :)
Created attachment 363896 [details] [review] msdkenc: fix handling YUY2,YV12,BGRA surface
Created attachment 363897 [details] [review] msdkenc: fix handling YUY2,UYVY,BGRA surface Based on slomo's patch.
Sebastian, could you test on this patch?
Looks weird to handle YUY2,UYVY,BGRA surface. I referenced to sample source code.
(In reply to Hyunjun Ko from comment #14) > (In reply to Sebastian Dröge (slomo) from comment #13) > > Also the conversion is done by VPP, so first converting YUY2 to NV12 and > > then passing it to the h264 encoder for example should work fine, or not? > > > > Or do you mean YUY2 is supposed to be *natively* handled by the JPEG encoder > > but not by others? > > No, I meant YUY2 -> NV12 -> encoding seems to be only for jpeg encoder. > This was according to the sample_encoder's help, which says "YUY2 is for > JPEG encode only." But I realized I'm wrong. That doesn't seem to make any sense :) If VPP can handle YUY2, it shouldn't matter if the encoder comes afterwards or not, and even less if the JPEG or another encoder comes afterwards. Either that comment is wrong, or what they mean is that the JPEG encoder can directly take YUY2 without conversion.
Review of attachment 363897 [details] [review]: This is necessary for anything to work? I assume this would also allow to support NV21, RGBA/BGRA/ARGB/ABGR then :)
commit 3c611da315f3755f4675fe4e874389e033f44d08 Author: Hyunjun Ko <zzoon@igalia.com> Date: Fri Nov 17 17:49:16 2017 +0900 msdkenc: Fix handling of YUY2, UYVY, BGRA surfaces https://bugzilla.gnome.org/show_bug.cgi?id=789847 commit d3eeb98f0ce298324f844c3678614afc27c3ce7d Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu Nov 16 11:32:52 2017 +0200 msdkenc: Add support for YV12, YUY2, UYVY and BGRA By doing conversion with VPP to NV12 before the actual encoding. https://bugzilla.gnome.org/show_bug.cgi?id=789847
IIUC, NV12 should be the only input format accepted by the encoders except for JPEG which can support yuy2 too. This is the same case for open source driver(one we use in gstremaer-vaapi) too. but open source driver can work with other input formats because there is an internal VPP for NV12 CSC inside the driver. I don't think msdk is doing it like this. AFAIK msdk is very strict on input format which should be Tiled NV12.
Doing conversion in the encoder, especially if so simple, makes it easier for applications to use the encoder. Also doing the conversion in the encoder is potentially faster as you can schedule both the conversion and encoding together (as is done here now) instead of waiting first for the conversion to finish.
Yes, current encoders in gst-msdk uses VPP if the color format is different from NV12. Remove this option and force to add a VPP element before the encoders would be a regression under the sight of many. Also, in te case of decoders, a VPP processing can be included in the srcpad buffers to offer more formats as output. But that's not the case currently. Another improvement to this approach would be to query VPP the supported color transformation when registering the elements. And later on, create encapsulate all the logic related to VPP so it could be reused by different elements.
Ok, So I guess we will be having 2 vpp paths in msdk elements. 1: implicit conversions in decoders and encoder 2: explicit conversion with vpp element. BTW, this reminds me of something I have noticed in gstremaer-vaapi + open source driver. The open source driver does CSC internally for all non-nv12 formats (and also for non-tiled nv12). But unfortunately, the code path of explicit vpp(which is using optimized 3D sampler) and the vpp path used by the driver internally(avs sampler) are different. Explicit vpp (vaapipostproc) seems to be more performant than the internal conversion . This is visible if you run parallel encode use cases.