GNOME Bugzilla – Bug 783862
omxh264enc: also use OMX_VIDEO_PARAM_AVCTYPE to set profile and level
Last modified: 2017-06-29 19:41:42 UTC
OMX provides a second API to define the profile and level. We can try using it as well so implementation won't have to rely on a specific one. I did some refactoring to make the set_format() shorter and easier to read. Feel free to squash those commits together before merging if you prefer.
Created attachment 353891 [details] [review] omxh264enc: factor out string to profile/level enum conversion
Created attachment 353892 [details] [review] omxh264enc: factor out update_param_profile_level()
Created attachment 353893 [details] [review] omxh264enc: set profile/level using OMX_VIDEO_PARAM_AVCTYPE as well The OMX specification defines two API to set the AVC profile and level: using OMX_VIDEO_PARAM_PROFILELEVELTYPE and OMX_VIDEO_PARAM_AVCTYPE. We were already supporting the former but not the latter. We are now setting both so implementation don't have to rely on a specific one.
Review of attachment 353893 [details] [review]: You need to initialize the structure with valid defaults (not just the profile and level), otherwise none of the Android component will work. Its not standard, but it would make this patch more useful.
(In reply to Nicolas Dufresne (stormer) from comment #4) > Review of attachment 353893 [details] [review] [review]: > > You need to initialize the structure with valid defaults (not just the > profile and level), otherwise none of the Android component will work. Its > not standard, but it would make this patch more useful. Shouldn't the get_parameter() call initialize those?
(In reply to Guillaume Desmottes from comment #5) > (In reply to Nicolas Dufresne (stormer) from comment #4) > > Review of attachment 353893 [details] [review] [review] [review]: > > > > You need to initialize the structure with valid defaults (not just the > > profile and level), otherwise none of the Android component will work. Its > > not standard, but it would make this patch more useful. > > Shouldn't the get_parameter() call initialize those? I think so, but Android component generally don't because Android generic code will do that already. In anycase, it makes the behaviour more consistent, as we'll rely on the same defaults everywhere instead of random defaults per component. What I got in my dirty branch, copied from Android code: https://paste.fedoraproject.org/paste/PwgMWes5qD1ONVMYrUJnjQ
And I forgot this part right before, that goes before caps neogotiation as it may be get overrided: param.nAllowedPictureTypes = OMX_VIDEO_PictureTypeI | OMX_VIDEO_PictureTypeP; param.eProfile = OMX_VIDEO_AVCProfileBaseline; /* will be overridden from caps */ param.eLevel = OMX_VIDEO_AVCLevel1; /* will be overridden from caps */ Android pick BaseLine + Level 1 as default. Probably that level should be set according to the width/height, even though, that I would expect the component to fix it (not sure if we read back the param at the end and make sure it matches the caps).
Overriding all the default settings isn't really an option for now as we don't provide gst properties to change them back. Furthermore the current gst-omx API with properties is to rely on component's defaults (0xffffffff=component default). So this should be done as a Android specific hack. No Android port has been merged so far so I'll just add a comment about it in the code. I also added a default profile/level as fallback if none is provided downstream.
Created attachment 353903 [details] [review] omxh264enc: set profile/level using OMX_VIDEO_PARAM_AVCTYPE as well The OMX specification defines two API to set the AVC profile and level: using OMX_VIDEO_PARAM_PROFILELEVELTYPE and OMX_VIDEO_PARAM_AVCTYPE. We were already supporting the former but not the latter. We are now setting both so implementation don't have to rely on a specific one. Also use a default level and profile if none has been specified downstream rather than using an undefined one.
Created attachment 354612 [details] [review] omxh264enc: set profile/level using OMX_VIDEO_PARAM_AVCTYPE as well The OMX specification defines two API to set the AVC profile and level: using OMX_VIDEO_PARAM_PROFILELEVELTYPE and OMX_VIDEO_PARAM_AVCTYPE. We were already supporting the former but not the latter. We are now setting both so implementation don't have to rely on a specific one.
I removed the default level/profile. It doesn't match with the general API of gst-omx which is default == component default. The default I picked weren't working on the pi. I don't see much point having platform specific defaults in gst-omx, best to rely on the OMX implementation which knows more about sensible default for its platform. These patches have been tested and are working on the pi.
Created attachment 354681 [details] [review] omxh264enc: factor out string to profile/level enum conversion
Created attachment 354682 [details] [review] omxh264enc: factor out update_param_profile_level()
Created attachment 354683 [details] [review] omxh264enc: set profile/level using OMX_VIDEO_PARAM_AVCTYPE as well The OMX specification defines two API to set the AVC profile and level: using OMX_VIDEO_PARAM_PROFILELEVELTYPE and OMX_VIDEO_PARAM_AVCTYPE. We were already supporting the former but not the latter. We are now setting both so implementation don't have to rely on a specific one.
Rebased on top of master.
Thanks. Attachment 354681 [details] pushed as af207f8 - omxh264enc: factor out string to profile/level enum conversion Attachment 354682 [details] pushed as 0dbe604 - omxh264enc: factor out update_param_profile_level() Attachment 354683 [details] pushed as 1fa2ea2 - omxh264enc: set profile/level using OMX_VIDEO_PARAM_AVCTYPE as well