tizonia / tizonia-openmax-il

Command-line cloud music player for Linux with support for Spotify, Google Play Music, YouTube, SoundCloud, TuneIn, iHeartRadio, Plex servers and Chromecast devices.
https://tizonia.org
GNU Lesser General Public License v3.0
1.69k stars 87 forks source link

libtizonia: deallocate_resources is dispatched before all FreeBuffer #364

Open CapOM opened 7 years ago

CapOM commented 7 years ago

From the spec: "On a transition from OMX_StateIdle to OMX_StateLoaded, when all of the buffers have been removed from the component, the state transition is complete."

Problem is that class->deallocate_resources is called as soon as the client calls setState(Loaded). So before that the client calls OMX_FreeBuffer.

Instead I think deallocate_resources should be called just before triggering OMX_EventCmdComplete event.

Indeed the component might still needs internal resources to properly free the buffers (if AllocateBuffer is custom). This is the case for the Mesa encoder. Currently it crashes at the end because when trying to FreeBuffer it accesses resources that have just been destroyed by class->deallocate_resources.

CapOM commented 7 years ago

So I workaround the issue with above commit. Though maybe tizonia deallocate_resources should be documented to say that it is called straight when client requested to go from Idle to Loaded, and not just before triggering CmdComplete. So most likely before any FreeBuffer those are still not destroyed.

juanrubio commented 7 years ago

Hi Julien

documentation is one of the largest gaps in the project (e.g. #131 and #128). Possibly I should start making that a priority!.

Regarding the original issue, your suggestion of revisiting the ordering of these events sounds like the right thing to do. But I want to do some analysis first, since there may be some hidden complexity to it.

For example, the way it works at the moment is:

Loaded -> Idle (resource allocation)

- Fsm notifies the kernel object first (buffers and OMX headers are allocated at this point, 
possibly in an async way, in either the local component, the tunneled component, the IL client 
or an arbitrary mix of all of the above).
- Next, the fsm notifies the processor object (resource allocation in this object is generally 
independent of other IL entities).
- Transition is completed when both objects call back on the FSM to inform that their resource 
allocation activities are finished. Callbacks are received in an arbitrary order (since the processor 
may finish before kernel).

and interestingly, the exact same ordering is applied during Idle -> Loaded

So it looks like the more sensible ordering of events would be:

Loaded -> Idle (resource allocation)

- Fsm notifies that the processor object first.
- When FSM receives the callback from the processor indicating allocation is complete, then
- Fsm notifies the kernel object.
- When FSM is notified that kernel allocation is complete, then
- Transition is completed.

and note that it would make sense to revert the above ordering during the Idle -> Loaded transition.

So I think in order to fix the Idle -> Loaded situation, the Loaded -> Idle should be fixed at the same time.

But... I need to analyse what other effects these changes may have in other parts of the state machine, e.g.

It may well be that nothing really breaks too much and that the changes can be applied without too much pain, but it is kind of difficult to see beforehand. The best way to see it is most likely to get on with it. Which I want to do sometime soon.