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.koe...@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
