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 794998 - omxvideoenc: restore OMX default target-bitrate if requested by user
omxvideoenc: restore OMX default target-bitrate if requested by user
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-omx
git master
Other Linux
: Normal enhancement
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-04-05 08:56 UTC by Guillaume Desmottes
Modified: 2018-04-23 08:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
omxvideoenc: factor out gst_omx_video_enc_set_bitrate() (4.82 KB, patch)
2018-04-05 08:57 UTC, Guillaume Desmottes
accepted-commit_now Details | Review
omxvideoenc: use gst_omx_video_enc_set_bitrate() when setting bitrate in set_format (1.77 KB, patch)
2018-04-05 08:57 UTC, Guillaume Desmottes
accepted-commit_now Details | Review
omxvideoenc: restore OMX default target-bitrate if requested by user (3.47 KB, patch)
2018-04-05 08:57 UTC, Guillaume Desmottes
none Details | Review
omxvideoenc: restore OMX default target-bitrate if requested by user (3.76 KB, patch)
2018-04-06 13:12 UTC, Guillaume Desmottes
none Details | Review

Description Guillaume Desmottes 2018-04-05 08:56:32 UTC
.
Comment 1 Guillaume Desmottes 2018-04-05 08:57:06 UTC
Created attachment 370549 [details] [review]
omxvideoenc: factor out gst_omx_video_enc_set_bitrate()

No semantic change, I'm about to re-use this function in set_format().
Comment 2 Guillaume Desmottes 2018-04-05 08:57:13 UTC
Created attachment 370550 [details] [review]
omxvideoenc: use gst_omx_video_enc_set_bitrate() when setting bitrate in set_format

We weren't using the usual pattern when re-setting the bitrate:
- get parameters from OMX
- update only the fields different from 0xffffffff (OMX defaults)
- set parameters

Also added a comment explaining why we re-set this param.
Comment 3 Guillaume Desmottes 2018-04-05 08:57:19 UTC
Created attachment 370551 [details] [review]
omxvideoenc: restore OMX default target-bitrate if requested by user

0xffffffff is the magic number in gst-omx meaning 'the default value
defined in OMX'. This works fine with OMX parameters which are only set
once when starting the component but not with configs which can be
changed while PLAYING.
Save the actual OMX default bitrate so we can restore it later if user
sets back 0xffffffff on the property.

Added GST_OMX_PROP_OMX_DEFAULT so we stop hardcoding magic numbers
everywhere.
Comment 4 Olivier Crête 2018-04-05 13:01:18 UTC
Review of attachment 370549 [details] [review]:

++
Comment 5 Olivier Crête 2018-04-05 13:01:32 UTC
Review of attachment 370550 [details] [review]:

++
Comment 6 Olivier Crête 2018-04-05 13:05:44 UTC
Review of attachment 370551 [details] [review]:

::: omx/gstomx.h
@@ +122,3 @@
+ * If set on a default_* variable means that the default values hasn't been
+ * retrieved from OMX yet. */
+#define GST_OMX_PROP_OMX_DEFAULT (0xffffffff)

Is this from the OMX spec? or is this only a local thing? Should't it be G_MAXUINT32 ?

::: omx/gstomxvideoenc.c
@@ +766,3 @@
+    if (self->default_target_bitrate == GST_OMX_PROP_OMX_DEFAULT)
+      /* Save the actual OMX default so we can restore it if needed */
+      self->default_target_bitrate = bitrate_param.eControlRate;

Can this default ever change if other controls are modified? Or is it set for the lifetime of the component even if stopped and restarted?

@@ +773,3 @@
       bitrate_param.nTargetBitrate = self->target_bitrate;
+    else
+      bitrate_param.nTargetBitrate = self->default_target_bitrate;

read from eControlRate but set to nTargetBitrate, is that voluntary?
Comment 7 Guillaume Desmottes 2018-04-06 13:12:44 UTC
(In reply to Olivier Crête from comment #6)
> Review of attachment 370551 [details] [review] [review]:
> 
> ::: omx/gstomx.h
> @@ +122,3 @@
> + * If set on a default_* variable means that the default values hasn't been
> + * retrieved from OMX yet. */
> +#define GST_OMX_PROP_OMX_DEFAULT (0xffffffff)
> 
> Is this from the OMX spec? or is this only a local thing? Should't it be
> G_MAXUINT32 ?

It's specific to gst-omx. We use this magic value all over the code so I decided to have a define for it.
See for example this property:
  target-bitrate      : Target bitrate in Kbps (0xffffffff=component default)

We use this explicit value so best to keep it as it rather than G_MAXUINT32 I think.

> ::: omx/gstomxvideoenc.c
> @@ +766,3 @@
> +    if (self->default_target_bitrate == GST_OMX_PROP_OMX_DEFAULT)
> +      /* Save the actual OMX default so we can restore it if needed */
> +      self->default_target_bitrate = bitrate_param.eControlRate;
> 
> Can this default ever change if other controls are modified? Or is it set
> for the lifetime of the component even if stopped and restarted?

It's up to the OMX implementation I guess.
Let's reset it in gst_omx_video_enc_stop() to be safe.

> @@ +773,3 @@
>        bitrate_param.nTargetBitrate = self->target_bitrate;
> +    else
> +      bitrate_param.nTargetBitrate = self->default_target_bitrate;
> 
> read from eControlRate but set to nTargetBitrate, is that voluntary?

No, that's a typo; good catch, thanks!
Comment 8 Guillaume Desmottes 2018-04-06 13:12:56 UTC
Created attachment 370578 [details] [review]
omxvideoenc: restore OMX default target-bitrate if requested by user

0xffffffff is the magic number in gst-omx meaning 'the default value
defined in OMX'. This works fine with OMX parameters which are only set
once when starting the component but not with configs which can be
changed while PLAYING.
Save the actual OMX default bitrate so we can restore it later if user
sets back 0xffffffff on the property.

Added GST_OMX_PROP_OMX_DEFAULT so we stop hardcoding magic numbers
everywhere.
Comment 9 Guillaume Desmottes 2018-04-20 10:59:49 UTC
Olivier: any other comment on this patch?
Comment 10 Olivier Crête 2018-04-20 17:40:46 UTC
(In reply to Guillaume Desmottes from comment #9)
> Olivier: any other comment on this patch?

Only thing left #define GST_OMX_PROP_OMX_DEFAULT G_MAXUINT32
Comment 11 Guillaume Desmottes 2018-04-23 08:42:53 UTC
Thanks, fixed and merged to master.

commit 72cb1943da2d28b8ec87d75d0821f6e480d2ef4d (HEAD -> master, origin/master, origin/HEAD)
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Thu Mar 29 16:42:40 2018 +0200

    omxvideoenc: restore OMX default target-bitrate if requested by user
    
    0xffffffff is the magic number in gst-omx meaning 'the default value
    defined in OMX'. This works fine with OMX parameters which are only set
    once when starting the component but not with configs which can be
    changed while PLAYING.
    Save the actual OMX default bitrate so we can restore it later if user
    sets back 0xffffffff on the property.
    
    Added GST_OMX_PROP_OMX_DEFAULT so we stop hardcoding magic numbers
    everywhere.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=794998

commit 542faf0f367a0dd06bf83bf124572a28b46dc04c
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Thu Mar 29 11:36:00 2018 +0200

    omxvideoenc: use gst_omx_video_enc_set_bitrate() when setting bitrate in set_format
    
    We weren't using the usual pattern when re-setting the bitrate:
    - get parameters from OMX
    - update only the fields different from 0xffffffff (OMX defaults)
    - set parameters
    
    Also added a comment explaining why we re-set this param.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=794998

commit 256f77b621777f7c961222059bf9cee37f22bd14
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Thu Mar 29 11:26:04 2018 +0200

    omxvideoenc: factor out gst_omx_video_enc_set_bitrate()
    
    No semantic change, I'm about to re-use this function in set_format().
    
    https://bugzilla.gnome.org/show_bug.cgi?id=794998