GNOME Bugzilla – Bug 791479
msdkenc: Support force-keyunit events
Last modified: 2018-02-22 20:34:20 UTC
See summary. It just has to checked on the input if GST_VIDEO_CODEC_FRAME_IS_FORCE_KEYFRAME(frame) and request a keyframe for that very input frame then.
I'm currently adding encoder tuning options which include the FORCE_KEYFRAME too. But if you already have the patch, just push it :)
Go ahead if it's simple to add to whatever other changes you already have :) Otherwise I'll get to it at some point
Created attachment 368685 [details] [review] msdk: enc: support force key unit event
Comment on attachment 368685 [details] [review] msdk: enc: support force key unit event >+ /* Force key-frame if needed */ >+ if (GST_VIDEO_CODEC_FRAME_IS_FORCE_KEYFRAME (input_frame)) >+ thiz->enc_cntrl.FrameType = >+ MFX_FRAMETYPE_I | MFX_FRAMETYPE_IDR | MFX_FRAMETYPE_REF; >+ else >+ thiz->enc_cntrl.FrameType = MFX_FRAMETYPE_UNKNOWN; Not that I know how this API works, but shouldn't this be just thiz->enc_cntrl.FrameType = MFX_FRAMETYPE_IDR; ?
Review of attachment 368685 [details] [review]: ::: sys/msdk/gstmsdkenc.c @@ +1393,3 @@ memset (&thiz->option2, 0, sizeof (thiz->option2)); memset (&thiz->option2, 0, sizeof (thiz->option3)); + memset (&thiz->enc_cntrl, 0, sizeof (thiz->enc_cntrl)); These memset and the FALSE assignation doesn't make sense, since the gobject allocation already clears to zero the object structure.
(In reply to Tim-Philipp Müller from comment #4) > Comment on attachment 368685 [details] [review] [review] > msdk: enc: support force key unit event > > >+ /* Force key-frame if needed */ > >+ if (GST_VIDEO_CODEC_FRAME_IS_FORCE_KEYFRAME (input_frame)) > >+ thiz->enc_cntrl.FrameType = > >+ MFX_FRAMETYPE_I | MFX_FRAMETYPE_IDR | MFX_FRAMETYPE_REF; > >+ else > >+ thiz->enc_cntrl.FrameType = MFX_FRAMETYPE_UNKNOWN; > > Not that I know how this API works, but shouldn't this be just > > thiz->enc_cntrl.FrameType = MFX_FRAMETYPE_IDR; > > ? Ideally yes, but the above code is what I have seen in msdk sample application. If a particular use case is already implemented in msdk sample application, I prefer to stick with same code since they (msdk developers) might have assumptions based on it :) I have seen cases like that for eg:https://github.com/Intel-Media-SDK/samples/issues/13
(In reply to Víctor Manuel Jáquez Leal from comment #5) > Review of attachment 368685 [details] [review] [review]: > > ::: sys/msdk/gstmsdkenc.c > @@ +1393,3 @@ > memset (&thiz->option2, 0, sizeof (thiz->option2)); > memset (&thiz->option2, 0, sizeof (thiz->option3)); > + memset (&thiz->enc_cntrl, 0, sizeof (thiz->enc_cntrl)); > > These memset and the FALSE assignation doesn't make sense, since the gobject > allocation already clears to zero the object structure. Thanks. I will update with another patch since those fields were introduced by a different patch.
Created attachment 368732 [details] [review] msdkenc: remove unnecessary memset and field clearing
Created attachment 368796 [details] [review] msdk: enc: support force key unit event
Created attachment 368797 [details] [review] msdkenc: remove unnecessary memset and field clearing
Review of attachment 368796 [details] [review]: Pushed as 3cc61f98b1f061cde714a22dab0d211de740aea6
Review of attachment 368797 [details] [review]: Pushed as 1c81bf4bdc67b518d6b3bbc22398c17fe8ced1da