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. * 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 vid_enc_SetParameter vid_enc_GetParameter vid_enc_SetConfig vid_enc_GetConfig > Signed-off-by: Christian König <christian.koe...@amd.com> > Signed-off-by: Leo Liu <leo....@amd.com> > --- > src/gallium/drivers/radeon/radeon_vce_40_2_2.c | 2 +- > src/gallium/state_trackers/omx/Makefile.am | 3 +- > src/gallium/state_trackers/omx/entrypoint.c | 9 +- > src/gallium/state_trackers/omx/vid_dec.c | 15 +- > src/gallium/state_trackers/omx/vid_enc.c | 853 > +++++++++++++++++++++++++ > src/gallium/state_trackers/omx/vid_enc.h | 81 +++ > 6 files changed, 954 insertions(+), 9 deletions(-) > create mode 100644 src/gallium/state_trackers/omx/vid_enc.c > create mode 100644 src/gallium/state_trackers/omx/vid_enc.h > [snip] > +OMX_ERRORTYPE vid_enc_LoaderComponent(stLoaderComponentType *comp) > +{ Better positioned goto statements will remove all of the conditionals in the error path. if (variable == NULL) and be rewritten as if (!variable), to be consistent with the rest of the code. > + /* 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) ? > +error: > + FREE(comp->name); > + > + if (comp->name_specific) { > + FREE(comp->name_specific[0]); > + FREE(comp->name_specific); > + } > + > + if (comp->role_specific) { > + FREE(comp->role_specific[0]); > + FREE(comp->role_specific); > + } > + > + return OMX_ErrorInsufficientResources; > +} > + > +static OMX_ERRORTYPE vid_enc_Constructor(OMX_COMPONENTTYPE *comp, OMX_STRING > name) > +{ > + vid_enc_PrivateType *priv; > + omx_base_video_PortType *port; > + struct pipe_screen *screen; > + OMX_ERRORTYPE r; > + int i; > + > + assert(!comp->pComponentPrivate); > + > + priv = comp->pComponentPrivate = CALLOC(1, sizeof(vid_enc_PrivateType)); > + if (!priv) > + return OMX_ErrorInsufficientResources; > + > + r = omx_base_filter_Constructor(comp, name); > + if (r) > + return r; > + > + priv->BufferMgmtCallback = vid_enc_BufferEncoded; > + priv->messageHandler = vid_enc_MessageHandler; > + priv->destructor = vid_enc_Destructor; > + > + comp->SetParameter = vid_enc_SetParameter; > + comp->GetParameter = vid_enc_GetParameter; > + comp->GetConfig = vid_enc_GetConfig; > + comp->SetConfig = vid_enc_SetConfig; > + > + priv->screen = omx_get_screen(); > + if (!priv->screen) > + return OMX_ErrorInsufficientResources; > + > + screen = priv->screen->pscreen; > + priv->s_pipe = screen->context_create(screen, priv->screen); > + if (!priv->s_pipe) > + return OMX_ErrorInsufficientResources; > + > + priv->t_pipe = screen->context_create(screen, priv->screen); > + if (!priv->t_pipe) > + return OMX_ErrorInsufficientResources; > + > + if (!vl_compositor_init(&priv->compositor, priv->s_pipe)) > + return OMX_ErrorInsufficientResources; > + > + if (!vl_compositor_init_state(&priv->cstate, priv->s_pipe)) > + return OMX_ErrorInsufficientResources; > + > + priv->sPortTypesParam[OMX_PortDomainVideo].nStartPortNumber = 0; > + priv->sPortTypesParam[OMX_PortDomainVideo].nPorts = 2; > + priv->ports = CALLOC(2, sizeof(omx_base_PortType *)); > + if (!priv->ports) > + return OMX_ErrorInsufficientResources; > + > + 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 ? > + priv->ports[i] = CALLOC(1, sizeof(omx_base_video_PortType)); > + if (!priv->ports[i]) > + return OMX_ErrorInsufficientResources; > + > + base_video_port_Constructor(comp, &priv->ports[i], i, i == 0); > + } > + > + port = (omx_base_video_PortType > *)priv->ports[OMX_BASE_FILTER_INPUTPORT_INDEX]; > + port->sPortParam.format.video.nFrameWidth = 176; > + port->sPortParam.format.video.nFrameHeight = 144; > + port->sPortParam.format.video.eColorFormat = > OMX_COLOR_FormatYUV420SemiPlanar; > + port->sVideoParam.eColorFormat = OMX_COLOR_FormatYUV420SemiPlanar; > + port->sPortParam.nBufferCountActual = 8; > + port->sPortParam.nBufferCountMin = 4; > + > + port->Port_SendBufferFunction = vid_enc_EncodeFrame; > + port->Port_AllocateBuffer = vid_enc_AllocateInBuffer; > + port->Port_UseBuffer = vid_enc_UseInBuffer; > + port->Port_FreeBuffer = vid_enc_FreeInBuffer; > + > + port = (omx_base_video_PortType > *)priv->ports[OMX_BASE_FILTER_OUTPUTPORT_INDEX]; > + strcpy(port->sPortParam.format.video.cMIMEType,"video/H264"); > + port->sPortParam.format.video.nFrameWidth = 176; > + port->sPortParam.format.video.nFrameHeight = 144; > + port->sPortParam.format.video.eCompressionFormat = OMX_VIDEO_CodingAVC; > + port->sVideoParam.eCompressionFormat = OMX_VIDEO_CodingAVC; > + > + port->Port_AllocateBuffer = vid_enc_AllocateOutBuffer; > + port->Port_FreeBuffer = vid_enc_FreeOutBuffer; > + > + priv->bitrate.eControlRate = OMX_Video_ControlRateDisable; > + priv->bitrate.nTargetBitrate = 0; > + > + priv->quant.nQpI = OMX_VID_ENC_QUANT_I_FRAMES_DEFAULT; > + priv->quant.nQpP = OMX_VID_ENC_QUANT_P_FRAMES_DEFAULT; > + priv->quant.nQpB = OMX_VID_ENC_QUANT_B_FRAMES_DEFAULT; > + > + priv->force_pic_type.IntraRefreshVOP = OMX_FALSE; > + priv->frame_num = 0; > + > + priv->scale.xWidth = OMX_VID_ENC_SCALING_WIDTH_DEFAULT; > + priv->scale.xHeight = OMX_VID_ENC_SCALING_WIDTH_DEFAULT; > + > + return OMX_ErrorNone; > +} > + [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. > + return OMX_ErrorNone; > +} > + > +static OMX_ERRORTYPE vid_enc_FreeOutBuffer(omx_base_PortType *port, OMX_U32 > idx, OMX_BUFFERHEADERTYPE *buf) > +{ > + OMX_COMPONENTTYPE* comp = port->standCompContainer; > + vid_enc_PrivateType *priv = comp->pComponentPrivate; > + > + if (buf->pOutputPortPrivate) { > + struct output_buf_private *outp = buf->pOutputPortPrivate; > + if (outp->transfer) > + pipe_transfer_unmap(priv->t_pipe, outp->transfer); > + pipe_resource_reference(&outp->bitstream, NULL); > + FREE(outp); > + buf->pOutputPortPrivate = NULL; > + } > + buf->pBuffer = NULL; > + > + return base_port_FreeBuffer(port, idx, buf); > +} > + > +static OMX_ERRORTYPE vid_enc_EncodeFrame(omx_base_PortType *port, > OMX_BUFFERHEADERTYPE *buf) > +{ > + OMX_COMPONENTTYPE* comp = port->standCompContainer; > + vid_enc_PrivateType *priv = comp->pComponentPrivate; > + struct input_buf_private *inp = buf->pInputPortPrivate; > + unsigned size = > priv->ports[OMX_BASE_FILTER_OUTPUTPORT_INDEX]->sPortParam.nBufferSize; > + OMX_VIDEO_PORTDEFINITIONTYPE *def = &port->sPortParam.format.video; > + struct pipe_h264_enc_picture_desc picture; > + struct pipe_video_buffer *vbuf = inp->buf; > + > + pipe_resource_reference(&inp->bitstream, NULL); > + > + if (buf->nFilledLen == 0) { > + if (buf->nFlags & OMX_BUFFERFLAG_EOS) > + buf->nFilledLen = buf->nAllocLen; > + return base_port_SendBufferFunction(port, buf); > + } > + > + if (buf->pOutputPortPrivate) { > + vbuf = buf->pOutputPortPrivate; > + } else { > + /* ------- load input image into video buffer ---- */ > + struct pipe_sampler_view **views; > + struct pipe_box box = {}; > + void *ptr; > + > + views = inp->buf->get_sampler_view_planes(vbuf); > + if (!views) > + return OMX_ErrorInsufficientResources; > + > + ptr = buf->pBuffer; > + > + box.width = def->nFrameWidth; > + box.height = def->nFrameHeight; > + box.depth = 1; > + > + priv->s_pipe->transfer_inline_write(priv->s_pipe, views[0]->texture, 0, > + PIPE_TRANSFER_WRITE, &box, > + ptr, def->nStride, 0); > + > + > + ptr = ((uint8_t*)buf->pBuffer) + (def->nStride * box.height); > + > + box.width = def->nFrameWidth / 2; > + box.height = def->nFrameHeight / 2; > + box.depth = 1; > + > + priv->s_pipe->transfer_inline_write(priv->s_pipe, views[1]->texture, 0, > + PIPE_TRANSFER_WRITE, &box, > + ptr, def->nStride, 0); > + } > + > + /* -------------- scale input image --------- */ > + > + if (priv->scale_buffer) { > + struct vl_compositor *compositor = &priv->compositor; > + struct vl_compositor_state *s = &priv->cstate; > + struct pipe_sampler_view **views; > + struct pipe_surface **dst_surface; > + unsigned i; > + > + views = vbuf->get_sampler_view_planes(vbuf); > + dst_surface = priv->scale_buffer->get_surfaces(priv->scale_buffer); > + vl_compositor_clear_layers(s); > + for (i = 0; i < VL_MAX_SURFACES; ++i) { > + if (!views[i] || !dst_surface[i]) > + continue; > + vl_compositor_set_rgba_layer(s, compositor, 0, views[i], NULL, > NULL, NULL); > + vl_compositor_render(s, compositor, dst_surface[i], NULL, false); > + } > + > + size = priv->scale.xWidth * priv->scale.xHeight * 2; > + vbuf = priv->scale_buffer; > + } > + > + priv->s_pipe->flush(priv->s_pipe, NULL, 0); > + > + /* -------------- allocate output buffer --------- */ > + > + inp->bitstream = pipe_buffer_create(priv->s_pipe->screen, > PIPE_BIND_VERTEX_BUFFER, > + PIPE_USAGE_STREAM, size); > + > + /* -------------- decode frame --------- */ > + > + switch (priv->bitrate.eControlRate) { > + case OMX_Video_ControlRateVariable: > + picture.rate_ctrl.rate_ctrl_method = > PIPE_H264_ENC_RATE_CONTROL_METHOD_VARIABLE; > + break; > + case OMX_Video_ControlRateConstant: > + picture.rate_ctrl.rate_ctrl_method = > PIPE_H264_ENC_RATE_CONTROL_METHOD_CONSTANT; > + break; > + case OMX_Video_ControlRateVariableSkipFrames: > + picture.rate_ctrl.rate_ctrl_method = > PIPE_H264_ENC_RATE_CONTROL_METHOD_VARIABLE_SKIP; > + break; > + case OMX_Video_ControlRateConstantSkipFrames: > + picture.rate_ctrl.rate_ctrl_method = > PIPE_H264_ENC_RATE_CONTROL_METHOD_CONSTANT_SKIP; > + break; > + default: > + picture.rate_ctrl.rate_ctrl_method = > PIPE_H264_ENC_RATE_CONTROL_METHOD_DISABLE; > + break; > + } > + > + 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 ? Cheers -Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev