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 748467 - vtenc: fix keyframe request race condition
vtenc: fix keyframe request race condition
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Mac OS
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-04-25 20:14 UTC by Ilya Konstantinov
Modified: 2015-06-01 11:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vtenc: fix keyframe request race condition (5.73 KB, patch)
2015-04-25 20:15 UTC, Ilya Konstantinov
none Details | Review
vtenc: fix keyframe request race condition (5.16 KB, patch)
2015-04-26 01:12 UTC, Ilya Konstantinov
none Details | Review
vtenc: fix keyframe request race condition (with comment) (5.63 KB, patch)
2015-04-27 00:07 UTC, Ilya Konstantinov
none Details | Review
vtenc: fix keyframe request race condition (w/mutable props) (4.88 KB, patch)
2015-04-27 23:09 UTC, Ilya Konstantinov
none Details | Review
vtenc: fix keyframe request race condition (5.16 KB, patch)
2015-05-05 00:21 UTC, Ilya Konstantinov
committed Details | Review

Description Ilya Konstantinov 2015-04-25 20:14:18 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.
Comment 1 Ilya Konstantinov 2015-04-25 20:15:44 UTC
Created attachment 302348 [details] [review]
vtenc: fix keyframe request race condition

Please review.
Comment 2 Tim-Philipp Müller 2015-04-25 21:49:31 UTC
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.
Comment 3 Ilya Konstantinov 2015-04-26 01:12:47 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2015-04-26 19:20:34 UTC
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.
Comment 5 Ilya Konstantinov 2015-04-26 20:00:44 UTC
> 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.
Comment 6 Ilya Konstantinov 2015-04-27 00:07:19 UTC
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? :)
Comment 7 Ilya Konstantinov 2015-04-27 23:09:32 UTC
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.
Comment 8 Ilya Konstantinov 2015-04-30 14:03:30 UTC
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.
Comment 9 Ilya Konstantinov 2015-05-05 00:21:16 UTC
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 };
Comment 10 Ilya Konstantinov 2015-05-31 16:54:54 UTC
slomo - ping :) If you're ok with it, please commit the (non-obsolete) patch.
Comment 11 Sebastian Dröge (slomo) 2015-06-01 11:32:27 UTC
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