[Bug 794998] omxvideoenc: restore OMX default target-bitrate if requested by user

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=794998

--- Comment #7 from Guillaume Desmottes <gdesmott at gnome.org> ---
(In reply to Olivier Crête from comment #6)
> 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 ?

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!

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.