Segmentation fault playing a recording with file missing

[Peter Bennett <pgbennett@…>]

Reported by Nicolas Riendeau, tracked down by Roger Siddons. This is caused by my fix for ticket #12691 I will submit a patch shortly. Note that ticket #12693 is also a fix for the same area of code, so the patch I submit will be dependent on #12693.

[Peter Bennett <pgbennett@…>]

Fix for this bug

[Peter Bennett <pgbennett@…>]

Note that this bug only occurs if a video has been deleted. It does not happen for deleted recording files.

Please commit the change for ticket #12693 before committing this change, otherwise you will have a conflict.

[Roger Siddons]

The video player (mythfrontend::main::internal_play_media()) plays video from any source. For instance Gallery uses it for camera videos and MythBrowser and SetupWizard use it for downloaded videos. So it has to fail in a generic/independent way.

"Try it and handle failure" is a more robust strategy than "Check first, do it and handle unexpected failure" (file disappears between check and play ?). It also avoids having to retrieve a remote file twice.

Recordings are a special case. The UI does extra checks on recording files so that they can be marked as missing in the UI. Therefore, it won't even try to play a recording that it already knows is absent.

[Peter Bennett <pgbennett@…>]

What you say makes sense.

I noted the fact that it does not fail on recordings, to inform and enable people who may be testing it or may encounter the problem.

Let me know if you have any problem with my patches.

[Roger Siddons]

Hi Peter,

Thanks for all your Pi work - I must get one sometime.

I was really responding to your IRC queries but we rarely seem to be around at the same time.

Now I see that #12693 would have prevented the original crash (but not on a Pi). This one looks like it solves it on the Pi too, but isn't isOpenMaxRender == true when NOT using openmax ?

IMO the dynamic cast is convoluted. Why not create your virtual GetName() ? It could default to an empty string and only needs to be overridden by videoout_omx. It may become useful for someone else later...

Then your change becomes a much simpler:

bool isOpenMaxRender = false;
if (ctx->m_player)
{
    VideoOutput *vo = ctx->player->GetVideoOutput();
    isOpenMaxRender = vo && vo->GetName() == "openmax";
}
if (!isOpenMaxRender && !weDisabledGUI)
    ...

[Peter Bennett <pgbennett@…>]

I will rework this fix, please do not commit the attached patch.

[Nicolas Riendeau]

Replying to rsiddons:

The video player (mythfrontend::main::internal_play_media()) plays video from any source. For instance Gallery uses it for camera videos and MythBrowser and SetupWizard use it for downloaded videos. So it has to fail in a generic/independent way.

"Try it and handle failure" is a more robust strategy than "Check first, do it and handle unexpected failure" (file disappears between check and play ?). It also avoids having to retrieve a remote file twice.

Recordings are a special case. The UI does extra checks on recording files so that they can be marked as missing in the UI. Therefore, it won't even try to play a recording that it already knows is absent.