GNOME Bugzilla – Bug 794998
omxvideoenc: restore OMX default target-bitrate if requested by user
Last modified: 2018-04-23 08:42:53 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().
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.
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.
Review of attachment 370549 [details] [review]: ++
Review of attachment 370550 [details] [review]: ++
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?
(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!
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.
Olivier: any other comment on this patch?
(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
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