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 793458 - omxvideoenc: make GST_PARAM_MUTABLE_PLAYING property thread safe
omxvideoenc: make GST_PARAM_MUTABLE_PLAYING property thread safe
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-omx
git master
Other Linux
: Normal normal
: 1.13.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-02-14 16:26 UTC by Guillaume Desmottes
Modified: 2018-02-21 16:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
omxvideoenc: protect target_bitrate with the object lock (3.60 KB, patch)
2018-02-14 16:26 UTC, Guillaume Desmottes
none Details | Review
omxvideoenc: remove GST_PARAM_MUTABLE_READY from 'max-bitrate' property (1.10 KB, patch)
2018-02-14 16:26 UTC, Guillaume Desmottes
needs-work Details | Review
omxvideoenc: protect target_bitrate with the object lock (3.60 KB, patch)
2018-02-21 08:59 UTC, Guillaume Desmottes
committed Details | Review
omxvideoenc: remove GST_PARAM_MUTABLE_PLAYING from 'max-bitrate' property (1.10 KB, patch)
2018-02-21 08:59 UTC, Guillaume Desmottes
committed Details | Review

Description Guillaume Desmottes 2018-02-14 16:26:22 UTC
.
Comment 1 Guillaume Desmottes 2018-02-14 16:26:47 UTC
Created attachment 368347 [details] [review]
omxvideoenc: protect target_bitrate with the object lock

The 'target-bitrate' property can be changed while PLAYING
(GST_PARAM_MUTABLE_PLAYING). Make it thread-safe to prevent concurrent
accesses between the application and streaming thread.
Comment 2 Guillaume Desmottes 2018-02-14 16:26:53 UTC
Created attachment 368348 [details] [review]
omxvideoenc: remove GST_PARAM_MUTABLE_READY from 'max-bitrate' property

This property isn't actually mutable in the PLAYING state.
Comment 3 Olivier Crête 2018-02-20 21:17:21 UTC
Review of attachment 368348 [details] [review]:

The patch loosk ok, but the title isn't.. you probably means PLAYING instead of ready there.
Comment 4 Guillaume Desmottes 2018-02-21 08:59:26 UTC
Indeed.
Comment 5 Guillaume Desmottes 2018-02-21 08:59:40 UTC
Created attachment 368692 [details] [review]
omxvideoenc: protect target_bitrate with the object lock

The 'target-bitrate' property can be changed while PLAYING
(GST_PARAM_MUTABLE_PLAYING). Make it thread-safe to prevent concurrent
accesses between the application and streaming thread.
Comment 6 Guillaume Desmottes 2018-02-21 08:59:44 UTC
Created attachment 368693 [details] [review]
omxvideoenc: remove GST_PARAM_MUTABLE_PLAYING from 'max-bitrate' property

This property isn't actually mutable in the PLAYING state.
Comment 7 Tim-Philipp Müller 2018-02-21 11:33:18 UTC
commit a8c3996317936c870e0f21d3dba2d64bba874418
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Wed Feb 14 17:23:39 2018 +0100

    omxvideoenc: remove GST_PARAM_MUTABLE_PLAYING from 'max-bitrate' property
    
    This property isn't actually mutable in the PLAYING state.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=793458

commit 2d3816fabc2d0951d814fa312895799116e94693
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Wed Feb 14 17:20:02 2018 +0100

    omxvideoenc: protect target_bitrate with the object lock
    
    The 'target-bitrate' property can be changed while PLAYING
    (GST_PARAM_MUTABLE_PLAYING). Make it thread-safe to prevent concurrent
    accesses between the application and streaming thread.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=793458
Comment 8 Nicolas Dufresne (ndufresne) 2018-02-21 16:04:32 UTC
Review of attachment 368692 [details] [review]:

::: omx/gstomxvideoenc.c
@@ +1015,3 @@
       break;
     case PROP_TARGET_BITRATE:
+      GST_OBJECT_LOCK (self);

Looks good, but in general we protect all properties regardless, so we don't forget when they get promoted.