Opened 4 years ago

Closed 4 years ago

#12709 closed Bug Report - Crash (Duplicate)

Segmentation fault playing a recording with file missing

Reported by: Peter Bennett <pgbennett@…> Owned by: JYA
Priority: minor Milestone: 0.28
Component: MythTV - Video Playback Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no

Description (last modified by Nicolas Riendeau)

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.

Attachments (1)

ticket_12709_fix_playback_segfault.patch (1.0 KB) - added by Peter Bennett <pgbennett@…> 4 years ago.
Fix for this bug

Download all attachments as: .zip

Change History (10)

Changed 4 years ago by Peter Bennett <pgbennett@…>

Fix for this bug

comment:1 Changed 4 years ago by 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.

comment:2 Changed 4 years ago by 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.

comment:3 Changed 4 years ago by 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.

comment:4 Changed 4 years ago by 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)
    ...

comment:5 Changed 4 years ago by Peter Bennett <pgbennett@…>

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

comment:6 in reply to:  2 Changed 4 years ago by Nicolas Riendeau

Description: modified (diff)
Milestone: unknown0.28

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.

comment:7 in reply to:  2 Changed 4 years ago by Nicolas Riendeau

Hi Roger!

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.

Assuming I correctly understood what you said I do not fully agree with this...

The null pointer check should be there regardless but it sounds to me like it is a null pointer because the file is missing so we already know before we reach that point that something is wrong.

Why should we rely on a side effect of protecting our code against coding errors and the like to handle something which should be handle long before starting to initialize the UI.

I have not tried Peter's patch but unless you tell me that with it in place the user is told what is actually wrong I do not consider that only handling the null pointer at this point is a good solution. From a programmer standpoint we "handled" the problem and did not crash but it would not be very user friendly.

If we want it to fail in a generic/independent way maybe we should actually remove all the code that does file handling from all of this function/method client's and (Gallery, MythBrowser?, etc...), make it common and make it handle all of the possible failures this should encounter.

Yes, we should protect our code against using null pointers but this should not be the only thing we do to handle those kind of errors...

I am very far from a C++ guru but in the languages I do program in I both program my code against using null pointers (even if it supposed to have been taken care of earlier) and try to catch the error as soon as we know it exists...

We are way too close to release to do little more than what Peter put in place however so this solution so Peter's current forthcoming solution will have to do...

Have a nice day!

Nicolas

comment:8 Changed 4 years ago by Peter Bennett <pgbennett@…>

I have changed the code to implement the strategy recommended by Roger Siddons and to fix this bug.

Please disregard the patch file above, and commit the revised patch file on ticket #12693.

comment:9 Changed 4 years ago by Stuart Auchterlonie

Resolution: Duplicate
Status: newclosed

Closing in favour of #12693 based on comment#8

Note: See TracTickets for help on using tickets.