Workaround for EVT_MEDIA_LOADED #1830
Conversation
Workaround for EVT_MEDIA_LOADED issue.
Workaround for EVT_MEDIA_LOADED issue.
Removed incorrect commit.
Fixed m_bLoadEventSent flag.
Fixed flag.. for the final time.
There was a problem hiding this comment.
I'm sorry, but this PR does a number of very non-obvious changes without any explanations, nor even instructions about reproducing the problem which it is supposed to fix. Could you please explain what's going on here? Without this we really can't do anything about it.
TIA!
| @@ -1590,11 +1600,10 @@ void wxAMMediaEvtHandler::OnActiveX(wxActiveXEvent& event) | |||
| // size of the video (will error) - however on 4 | |||
| // it won't play on downloaded things until it is | |||
| // completely downloaded so we use the lesser of two evils... | |||
| else if(event[0].GetInteger() == 3 && | |||
| else if(event[0].GetInteger() >= 3 && | |||
There was a problem hiding this comment.
The comment above explicitly says that we don't want to do it on 4, so at the very least it needs to be changed/updated to avoid confusion. But it's not clear at all whether you decided it wasn't the lesser evil, finally, or if you think this problem doesn't exist any more or something else. Please explain it.
There was a problem hiding this comment.
The problem was that the video wouldn't play if the load event is sent on 3. This is solved by in essence delaying the sending of the load event. When the video finished loading, the AM software sends a stop event. The "workaround" takes advantage of this and sends the finish load event if it's been triggered by receiving event 4.
To summarize, the FinishLoad has been broken down into a two part operation to avoid the problem with not being able to determine the video size, while still allowing event 4 to trigger the calling of FinishLoad. In the DirectShow backend, there is no notification when the video is fully loaded.
| !m_bLoadEventSent) | ||
| { | ||
| m_bLoadEventSent = true; | ||
| m_amb->FinishLoad(); |
There was a problem hiding this comment.
This really seems wrong, why are we setting m_bLoadEventSent to true if we're not actually sending the event?
There was a problem hiding this comment.
Calling m_amb->FinishLoad is sending the load event. The meaning of the flag has been changed to load event queued. m_bLoadEventSent could be renamed, to m_bLoadEventQueued, if that would improve the situation.
| m_bLoadEventSent = false; | ||
|
|
||
| m_amb->FinishLoad(); |
There was a problem hiding this comment.
This just doesn't make sense, you seem to have changed the meaning of m_bLoadEventSent flag without changing its name nor the comments describing it. A flag with this name must be set to true, not false, when FinishLoad() is called.
There was a problem hiding this comment.
Renaming m_bLoadEventSent to m_bLoadEventQueued would probably clear up the misunderstanding, but I wanted to fit the solution as closely as possible with the existing code base, changing as little as possible.
There was a problem hiding this comment.
Could you please update the PR to result in clear code and also add the comment(s) explaining what you wrote above?
Making as few changes as possible is certainly laudable, but it should be a secondary change, the primary ones should be to have correct and as simple/clear possible code.
TIA!
|
This PR has been in "work needed" state for more than a year and will be closed soon. Please update it, e.g. by commenting here, if anybody intends to resume working on it. |
|
If anybody is using |
This uses the stop event sent after a video is loaded to indicate it has completed loaded.