Opened 12 years ago

Closed 12 years ago

#3963 closed defect (fixed)

[PATCH] Use DTVRecorder::Reset() instead of derived class' Reset()

Reported by: Shane Shrybman <gnome42@…> Owned by: danielk
Priority: minor Milestone: unknown
Component: mythtv Version: head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

In livetv with DTV recorders we incorrectly delete the positionmap when we transition from one show to the next.

During transition:

RecorderBase::CheckForRingBufferSwitch?(void) calls ResetForNewFile?() before the curRecording is set to the new recording. DTVRecorder::ResetForNewFile?() calls the base class's Reset() which will delete the positionmap from the DB for the curRecording. If we call DTVRecorder::Reset() instead we won't delete the postionmap from the DB and we have the same behaviour as the non-DTV recorders.

Attachments (2)

dtv_ResetForNewFile.diff (527 bytes) - added by Shane Shrybman <gnome42@…> 12 years ago.
DTVRecorder::Reset() patch
3963-v1.patch (2.5 KB) - added by danielk 12 years ago.
Patch based on Shane's mythtv_multirec_dvbrec_ResetForNewFile.diff

Download all attachments as: .zip

Change History (10)

Changed 12 years ago by Shane Shrybman <gnome42@…>

Attachment: dtv_ResetForNewFile.diff added

DTVRecorder::Reset() patch

comment:1 Changed 12 years ago by danielk

Milestone: unknown0.21
Owner: changed from Isaac Richards to danielk
Type: defectpatch
Version: unknownhead

comment:2 Changed 12 years ago by Shane Shrybman <gnome42@…>

oops, I meant derived class of course :) ( DTVRecorder::ResetForNewFile??() calls the derived class' Reset() which ...)

Yet another way to solve this problem. Make DTVRecorder::ResetForNewFile?() do nothing.:)

I like this better because it solves the posmap problem and cleans up the redundant Reset's that occur with DTV recorders.

Because DTVRecorder::ResetForNewFile?(void) does one thing only, calls Reset(), we end up doing redundant Reset()'s in the case of DTVRecorders when Reset() is called after ResetForNewFile?(void).

this happens in DTV livetv:

TVRec::TuningRestartRecorder?(void)

TVRec::SwitchLiveTVRingBuffer()

RecorderBase::CheckForRingBufferSwitch?(void)

DTVRecorder::ResetForNewFile?() -> recorder's Reset() which calls DTVRecorder::Reset(), and also deletes the posmap for the curRecording from the DB. This is too early, wrong posmap.

... then back up in TuningRestartRecorder?(void), after the curRecording has been set to the new one, we call

recorder->Reset()

Which for DTVRecorders means calling DTVRecorder::Reset(), and doing a full reset again. But the timing is better. :)

So if DTVRecorder::ResetForNewFile?() does nothing instead of calling Reset() it won't delete the posmap for the old curRecording and it won't duplicate recorder->Reset(). For the DTV Recorders that don't define a Reset() they will also benefit by getting only one DTVRecorder::Reset() called instead of two.

I think livetv is the only place that DTVRecorder::ResetForNewFile?() is ever used.

Shane

comment:3 Changed 12 years ago by danielk

Milestone: 0.21unknown
Type: patchdefect

The patch is incorrect. We should be calling the overridden Reset(). The problem is really the Reset() in DVB Recorder which is at issue. Why is it clearing the position map? My guess is it has to do with how we do channel changing with the DVB Recorder.

Changed 12 years ago by danielk

Attachment: 3963-v1.patch added

Patch based on Shane's mythtv_multirec_dvbrec_ResetForNewFile.diff

comment:4 Changed 12 years ago by cal@…

With 3963-v1.patch and --enable-dvb, the build fails ...

dvbrecorder.cpp:828: error: no ‘void DVBRecorder::Reset()’ member function declared in class ‘DVBRecorder’

comment:5 Changed 12 years ago by danielk

(In [14615]) Refs #3963. Applies DTVRecorder reset patch to multirec.

The problem is that the DVB recorder and some other DTVRecorder based recorders lose the positionmap on LiveTV transitions. This fixed the problem in the multirec branch. (Fixing this properly in trunk would be more complicated so we haven't attempted that, hopefully the multirec branch will be ready for merging to trunk soon.)

comment:6 Changed 12 years ago by danielk

(In [14616]) Refs #3963. Implements caching of ProgramInfo::IsSameProgram?() for scheduler.

This provides about a 10% speedup in exhange for a few kilobytes more RAM usage during scheduling. I also tested caching IsSameProgramTimeslot?() but it did not provide for a measurable speedup in scheduling.

comment:7 Changed 12 years ago by danielk

Oops, [14616] should have referenced #3326.

comment:8 Changed 12 years ago by danielk

Resolution: fixed
Status: newclosed

Fixed by multirec merge.

Note: See TracTickets for help on using tickets.