Opened 6 years ago

Closed 5 years ago

#11652 closed Patch - Bug Fix (Fixed)

Fix DXVA segfault upon playback exit

Reported by: SHinck@… Owned by: dblain
Priority: minor Milestone: 0.28
Component: MythTV - Video Playback Version: 0.26-fixes
Severity: medium Keywords:
Cc: Ticket locked: no

Description

I've found the reason for the crashes on video exit and fixed it: That's because IDirectXVideoDecoderService_CreateSurface creates surface_count + 1 surfaces (1 frontbuffer + surface_count backbuffers) but IDirectXVideoDecoderService_CreateDecoder is created with surface_count buffers.

I also fixed the significant corruption when playback starts and following a skip: That's because of begin_frame in dxva2.c decoder fails. This patch fixes this.

I've attached the two patches for fixes/0.26

Attachments (3)

0001-DXVA2_stabilization.commit (2.5 KB) - added by Raymond Wagner 6 years ago.
0001-fix_dxva2_segfaulting.commit (2.2 KB) - added by Raymond Wagner 6 years ago.
Version.txt (420 bytes) - added by Raymond Wagner 6 years ago.

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by Raymond Wagner

Changed 6 years ago by Raymond Wagner

Changed 6 years ago by Raymond Wagner

Attachment: Version.txt added

comment:1 Changed 6 years ago by stuartm

Milestone: unknown0.27
Owner: set to stuartm
Status: newaccepted

The segfault patch seems to contain more changes than simply fixing the off-by-one error. Can you explain those, and maybe move them into a separate patch?

comment:2 Changed 6 years ago by stuartm

Status: acceptedinfoneeded

comment:3 Changed 6 years ago by SHinck@…

According to Microsoft MSDN The CreateSurface? method also adds reference counts to the surface buffers: "The method fills the array with IDirect3DSurface9 pointers. The caller must release all of the interface pointers. In addition, the front buffer holds a reference count on each of the back buffers. Therefore, the back buffers are never deleted until the front buffer is deleted."

So I eliminated the additional add of reference counts to all the buffers because this would lead to a Memory leak because our code releases this buffers only once.

Additionally I designed the release of this buffers as SafeRelease? which is recommended by Microsoft: "

  1. Checks whether the pointer is NULL.
  2. Calls Release if the pointer is not NULL.
  3. Sets the pointer to NULL.

"

The change in the desired bitstream type to 2 perhaps should removed from this patch. Background of this is also discussed on MSDN: " For DXVA H.264/Mpeg-4 AVC

The value 2 for bConfigBitstreamRaw has the same meaning as the value 1, except as follows: – If bConfigBitstreamRaw is equal to 2, the slice control data structure that is used is the DXVA_Slice_H264_Short data structure. – Otherwise (bConfigBitstreamRaw is not equal to 2), the slice control data structure that is used is the DXVA_Slice_H264_Long data structure. " Type 2 should lead to smoother transitions at seeking in contents.

comment:4 Changed 6 years ago by Sascha Hinck <SHinck@…>

In 9406c5ba7589c3317257ed954e1e0a5442ba31e8/mythtv:

Fix segfaulting with DXVA2 hardware decoding at player end

If playing content with DXVA2 hardware decoding mythfrontend segfaults
on player end. That's because IDirectXVideoDecoderService_CreateSurface
creates surface_count + 1 surfaces (1 frontbuffer + surface_count
backbuffers) but IDirectXVideoDecoderService_CreateDecoder is created with
surface_count buffers.

Refs #11652

Signed-off-by: Stuart Morgan <smorgan@…>

comment:5 Changed 6 years ago by Sascha Hinck <SHinck@…>

In d441bdf5a083e27e7def6a7c0ac205e071e63c45/mythtv:

Fix Memory Leak in DVXA

According to Microsoft MSDN The CreateSurface?? method also adds reference counts to the surface buffers:

"The method fills the array with IDirect3DSurface9 pointers. The caller must release all of the interface pointers. In addition, the front buffer holds a reference count on each of the back buffers. Therefore, the back buffers are never deleted until the front buffer is deleted."

So I eliminated the additional add of reference counts to all the buffers because this would lead to a Memory leak because our code releases this buffers only once.

Additionally I designed the release of this buffers as SafeRelease?? which is recommended by Microsoft:

Checks whether the pointer is NULL.
Calls Release if the pointer is not NULL.
Sets the pointer to NULL.

Refs #11652

Signed-off-by: Stuart Morgan <smorgan@…>

comment:6 Changed 6 years ago by stuartm

Milestone: 0.270.28
Owner: changed from stuartm to dblain
Status: infoneededassigned

I have committed the segfault and memory leak fixes.

I've not applied the 'stabilisation' patch as that really should be included upstream in ffmpeg first. We can always cherry-pick this back to 0.27-fixes/0.27.1 once they've accepted the changes or made their own.

I've also not included the bitstream change as I believe that should get some testing from a dev who has a windows frontend.

comment:7 Changed 5 years ago by Karl Egly

Resolution: Fixed
Status: assignedclosed

Thank you for your contribution. I'm closing this ticket as fixed as the fix for the crash was committed 15 months ago. Please consider contributing the DVXA patch upstream if you have not done so already.

Note: See TracTickets for help on using tickets.