GNOME Bugzilla – Bug 748467
vtenc: fix keyframe request race condition
Last modified: 2015-06-01 11:32:27 UTC
It is incorrect to modify the frame properties after passing them, since VTCompressionSessionEncodeFrame takes reference and we have no control over when it's being used. (For example, when asking to encode frame 17, we'd modify the props to request keyframe, submit the frame (and props), then immediately revert the frame props back. When frame 17's time came, the encoder would not make it a keyframe.) In fact, the code can be simplified. We just preallocate the frame properties needed for keyframe requests, use use this single instance. When keyframe is not needed, we just pass NULL.
Created attachment 302348 [details] [review] vtenc: fix keyframe request race condition Please review.
Comment on attachment 302348 [details] [review] vtenc: fix keyframe request race condition You need to chain up to the parent class in the finalize function.
Created attachment 302355 [details] [review] vtenc: fix keyframe request race condition Oops, now chaining parent class in finalize. Also removed stray debug code that somehow ended up in the previous patch.
It would seem better to create new options there whenever we have to create a keyframe. We might want to set other things in the options at a later time that persists over multiple frames, and then would have to merge those two. Keeping these transient options around all the time seems a bit weird.
> We might want to set other things in the options at a later time that persists over multiple frames When we do, we can change it. Until then, it's just unneeded allocs per-frame. I switched to a non-mutable CFDictionaryRef to protect from unintentional misuse. If you insist, I can keep an empty 'CFMutableDictionaryRef frame_props', and do CFDictionaryCreateMutableCopy within gst_vtenc_encode_frame. My (personal) coding style goes against code that prepares for a future that might never come. Such code also has the drawback of giving the reader a false impression that something is used for more than it really is.
Created attachment 302405 [details] [review] vtenc: fix keyframe request race condition (with comment) I've added a comment for sake of future frame properties. Is it an acceptable compromise? :)
Created attachment 302487 [details] [review] vtenc: fix keyframe request race condition (w/mutable props) Another variant -- frame properties can be updated in gst_vtenc_update_frame_props. No doing any allocations in gst_vtenc_encode_frame. This variant is the most flexible, but it's all a moot point -- kVTEncodeFrameOptionKey_ForceKeyFrame is the only option there is (just went thru VT headers), and tbh attachment 302355 [details] [review] seems the simplest.
Made up my mind and obsoleted the newer patches. Since kVTEncodeFrameOptionKey_ForceKeyFrame is the only possible frame property at the moment, the original patch is the most appropriate.
Created attachment 302903 [details] [review] vtenc: fix keyframe request race condition Oops, yet again I end up rushing a patch before even testing :( + CFBooleanRef keyframe_props_values = { kCFBooleanTrue }; to + CFBooleanRef keyframe_props_values[] = { kCFBooleanTrue };
slomo - ping :) If you're ok with it, please commit the (non-obsolete) patch.
commit 94e7ed132346b7a9b60dfbc7f53ea18cf50abf8d Author: Ilya Konstantinov <ilya.konstantinov@gmail.com> Date: Sat Apr 25 22:55:28 2015 +0300 vtenc: fix keyframe request race condition It is incorrect to modify the frame properties after passing them, since VTCompressionSessionEncodeFrame takes reference and we have no control over when it's being used. In fact, the code can be simplified. We just preallocate the frame properties for keyframe requests, and pass NULL otherwise. https://bugzilla.gnome.org/show_bug.cgi?id=748467