[Mesa-dev] [PATCH 4/4] st/omx: initial OpenMAX H264 encoder v6

[Emil Velikov]

On 11/02/14 19:30, Christian König wrote:
> Am 11.02.2014 19:56, schrieb Emil Velikov:
>> On 11/02/14 17:27, Christian König wrote:
>>> From: Christian König <christian.koenig at amd.com>
>>>
>>> v2 (chk): fix eos handling
>>> v3 (leo): implement scaling configuration support
>>> v4 (leo): fix bitrate bug
>>> v5 (chk): add workaround for bug in Bellagio
>>> v6 (chk): fix div by 0 if framerate isn't known,
>>>            user separate pipe object for scale and transfer,
>>>            always flush the transfer pipe before encoding
>>>
>> Hi Christian
>>
>> Not familiar with the API, so a rather naive question:
>>
>>   * If vid_enc_Constructor fails I'm assuming that vid_enc_Destructor
>> will be executed to clean-up the mess.
> 
> To be honest I couldn't confirm that so far. I just followed the example
> code from Bellagio. Going to double check that, better save than sorry.
> 
>>   * The following four functions have some massive switch statements,
>> possibly consider
>>   - Adding a idx to variable type map
>>   - Move checkHeader(param, map[idx]) before the switch
>>   - Finally drop the brakets in the case statements
> 
> Considered that as well, but that doesn't work the because common values
> for idx are 0x02000000-0x02000015,  0x03000000- 0x0300001a,
> 0x05000000-.... etc.
> 
I feared that was the case. Alternative could be struct foo* with a loop
that checks for the idx and returns the (variable) type.

Might be an over kill, depending on how much one likes big switch
statements.

>> [snip]
>>> +OMX_ERRORTYPE vid_enc_LoaderComponent(stLoaderComponentType *comp)
>>> +{
>> Better positioned goto statements will remove all of the conditionals in
>> the error path.
> 
> Good point going to change that.
> 
>> if (variable == NULL) and be rewritten as if (!variable), to be
>> consistent with the rest of the code.
> 
> That's coding style of the example code vs. mesa coding style. I've
> already changed the indention so yeah probably a good idea to change
> that as well.
> 
Noticed that the coding style differs slightly, and thanks for sticking
with my comments.

>>> +   /* component 1 - video encoder */
>>> +   comp->componentVersion.s.nVersionMajor = 0;
>>> +   comp->componentVersion.s.nVersionMinor = 0;
>>> +   comp->componentVersion.s.nRevision = 0;
>>> +   comp->componentVersion.s.nStep = 1;
>>> +   comp->name_specific_length = 1;
>>> +
>>> +   comp->name = CALLOC(1, OMX_MAX_STRINGNAME_SIZE);
>>> +   if (comp->name == NULL)
>>> +      goto error;
>>> +
>>> +   comp->name_specific = CALLOC(comp->name_specific_length,
>>> sizeof(char *));
>>> +   if (comp->name_specific == NULL)
>>> +      goto error;
>>> +
>>> +   comp->name_specific[0] = CALLOC(1, OMX_MAX_STRINGNAME_SIZE);
>>> +   if (comp->name_specific[0] == NULL)
>>> +      goto error;
>>> +
>>> +   comp->role_specific = CALLOC(comp->name_specific_length,
>>> sizeof(char *));
>>> +   if (comp->role_specific == NULL)
>>> +      goto error;
>>> +
>>> +   comp->role_specific[0] = CALLOC(1, OMX_MAX_STRINGNAME_SIZE);
>>> +   if (comp->role_specific[0] == NULL)
>>> +      goto error;
>>> +
>>> +   vid_enc_name(comp->name);
>>> +   vid_enc_name_avc(comp->name_specific[0]);
>>> +
>>> +   strcpy(comp->role_specific[0], OMX_VID_ENC_AVC_ROLE);
>>> +
>>> +   comp->constructor = vid_enc_Constructor;
>>> +
>>> +   return 2;
>>> +
>> Why do we return 2, as there is only one component (ie the encoder) ?
> 
> Looks like a bug to me. Probably just copy & paste from the decoder.
> 
Speaking of bugs, why does omx_component_library_Setup return 2
components when both vid_dec_LoaderComponent and vid_enc_LoaderComponent
return zero (OMX_ErrorNone).

AFAICS the API does not have a recommendation but I'm inclined to
believe that returning 0 should be the better solution. As one does give
the total number of components whenever stComponents == NULL.

Cheers
-Emil

>>> +
>>> +   for (i = 0; i < 2; ++i) {
>> Is is possible for a encoder to have more than two number of ports ?
>> Would it be worth adding a define ?
> 
> Nope, one input, one output. I can't come up with a good use for
> anything else.
> 
>>
>> [snip]
>>> +static OMX_ERRORTYPE vid_enc_AllocateOutBuffer(omx_base_PortType
>>> *port, OMX_INOUT OMX_BUFFERHEADERTYPE **buf,
>>> +                                               OMX_IN OMX_U32 idx,
>>> OMX_IN OMX_PTR private, OMX_IN OMX_U32 size)
>>> +{
>>> +   OMX_ERRORTYPE r;
>>> +
>>> +   r = base_port_AllocateBuffer(port, buf, idx, private, size);
>>> +   if (r)
>>> +      return r;
>>> +
>>> +   FREE((*buf)->pBuffer);
>>> +   (*buf)->pBuffer = NULL;
>>> +   (*buf)->pOutputPortPrivate = CALLOC(1, sizeof(struct
>>> output_buf_private));
>>> +
>> I know I'm rather pedantic but can we handle CALLOC failure, please.
> 
> Missed that one, going to change it.
> 
>>> +
>>> +   if (picture.rate_ctrl.rate_ctrl_method !=
>>> PIPE_H264_ENC_RATE_CONTROL_METHOD_DISABLE) {
>>> +      if (priv->bitrate.nTargetBitrate < OMX_VID_ENC_BITRATE_MIN)
>>> +         picture.rate_ctrl.target_bitrate = OMX_VID_ENC_BITRATE_MIN;
>>> +      else if (priv->bitrate.nTargetBitrate < OMX_VID_ENC_BITRATE_MAX)
>>> +         picture.rate_ctrl.target_bitrate =
>>> priv->bitrate.nTargetBitrate;
>>> +      else
>>> +         picture.rate_ctrl.target_bitrate = OMX_VID_ENC_BITRATE_MAX;
>>> +      picture.rate_ctrl.peak_bitrate =
>>> picture.rate_ctrl.target_bitrate;
>>> +      picture.rate_ctrl.frame_rate_den =
>>> OMX_VID_ENC_CONTROL_FRAME_RATE_DEN_DEFAULT;
>>> +      picture.rate_ctrl.frame_rate_num = ((priv->frame_rate) >> 16)
>>> * picture.rate_ctrl.frame_rate_den;
>>> +      if (picture.rate_ctrl.target_bitrate <
>>> OMX_VID_ENC_BITRATE_MEDIAN)
>>> +         picture.rate_ctrl.vbv_buffer_size =
>>> MIN2((picture.rate_ctrl.target_bitrate * 2.75),
>>> OMX_VID_ENC_BITRATE_MEDIAN);
>>> +      else
>>> +         picture.rate_ctrl.vbv_buffer_size =
>>> picture.rate_ctrl.target_bitrate;
>>> +
>>> +      if (picture.rate_ctrl.frame_rate_num) {
>>> +         unsigned long long t = picture.rate_ctrl.target_bitrate;
>>> +         t *= picture.rate_ctrl.frame_rate_den;
>>> +         picture.rate_ctrl.target_bits_picture = t /
>>> picture.rate_ctrl.frame_rate_num;
>>> +      } else {
>>> +         picture.rate_ctrl.target_bits_picture =
>>> picture.rate_ctrl.target_bitrate;
>>> +      }
>>> +      picture.rate_ctrl.peak_bits_picture_integer =
>>> picture.rate_ctrl.target_bits_picture;
>>> +      picture.rate_ctrl.peak_bits_picture_fraction = 0;
>> Use local variable to store picture.rate_ctrl, to ease the eyes ?
> 
> Good idea.
> 
> Thx for the review,
> Christian.
> 
>> Cheers
>> -Emil
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>