Closed gpalsingh closed 7 years ago
Reverting to https://github.com/gpalsingh/mesa/commit/67c6e145fc987096ea3e6527ba5c52618789c7ed still gives that error which used to work earlier. Could related to gstreamer instead.
I found this is because deallocate is called before FreeBuffer during the transition state from Idle to Loaded. So it looks to me to be an issue in Tizonia, I reported to Juan here https://github.com/tizonia/tizonia-openmax-il/issues/364
Waiting for his feedback.
Hi guys,
I think understand why changing the ordering (FreeBuffer vs prc_deallocate_resources) would help in this specific case. But I wonder if we can have another scenario in the future that breaks because the ordering is the opposite to what that scenario needs.
So in other words, can't this scenario be made to work with the current ordering?.
e.g.: here, the pipe should be assigned to NULL after destruction:
if (p_prc->t_pipe) {
p_prc->t_pipe->destroy(p_prc->t_pipe);
p_prc->t_pipe = NULL; // <--- to make it clear that this resource no longer exists
}
And the FreeBuffer implementation maybe could take this into account:
if (outp) {
if (outp->transfer && p_prc->t_pipe)
pipe_transfer_unmap(p_prc->t_pipe, outp->transfer);
pipe_resource_reference(&outp->bitstream, NULL);
FREE(outp);
buf->pOutputPortPrivate = NULL;
}
buf->pBuffer = NULL;
I think I would need to understand better how the encoder port and the processor are sharing their resources. But maybe something along these lines would solve the problem?
Thx for the suggestion. I thought about it but then it will leak the buffer as it cannot properly release if it is still mapped.
Another solution would be to add prc_deallocate_resources_after ? (and rename prc_deallocate_resources to prc_deallocate_resources_before), something like that so for more flexibility
I think changing the ordering is probably the ideal solution. The problem is that adding another framework API to libtizonia or changing the ordering of events in such deep internals is (potentially) a major risk/effort.
I still believe this is solvable at component level (of course, at the expense of increasing a little bit the final LoC in the component):
So here is another idea:
#ifndef H264EPRC_H
#define H264EPRC_H
void * h264e_prc_class_init(void * ap_tos, void * ap_hdl);
void * h264e_prc_init(void * ap_tos, void * ap_hdl);
OMX_ERRORTYPE h264e_prc_free_buffer_resources(void *ap_obj,
struct pipe_transfer *transfer,
struct pipe_resource *bitstream);
#endif /* H264EPRC_H */
So the encoder processor could expose a new 'free_buffer_resources' to deallocate those resources that have buffer dependencies. FreeBuffer would call 'free_buffer_resources' on the processor class, to unmap and release the buffer. The processor then maintains a reference count, and when this reaches 0, the pipe resource is destroyed. 'prc_deallocate_resources' could be kept to deallocate those resources that have no dependencies with existing buffers.
if (outp) {
if (outp->transfer) {
h264e_prc_t *p_prc = tiz_get_prc(ap_hdl);
h264e_prc_free_buffer_resources(p_prc, outp->transfer, &outp->bitstream);
}
FREE(outp);
buf->pOutputPortPrivate = NULL;
}
buf->pBuffer = NULL;
It is not ideal but it is doable. Also in this case I think we should just move the pipe_t creation/destruction to h264eprc constructor/destructor. That should be easier. Thx
Hi Gurkirpal, please cherry-pick my patch https://github.com/CapOM/mesa/commit/fe40ffa0112783340708e3f2d3bc9461761a5e96 . It fixes the issue.
Works. Thx!