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

[Christian König]

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.

> [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.

>> +   /* 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.

>> +
>> +   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