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