Opened 9 years ago
Closed 4 years ago
#12290 closed Bug Report - General (Fixed)
Commercial Flagging isn't being queued.
Reported by: | Owned by: | Roger Siddons | |
---|---|---|---|
Priority: | minor | Milestone: | 30.1 |
Component: | MythTV - General | Version: | Master Head |
Severity: | medium | Keywords: | |
Cc: | Ticket locked: | no |
Description
In: TVRecEvent programinfo.cpp (GetFilesize?) the existing returned filesize is sometimes 0 and TVRec::FinishedRecording? won't allow the job to be queued if the file size is less than 1000 bytes.
This has deactivated all commercial flagging. A workaround is attached (not a fix.)
Attachments (8)
Change History (43)
Changed 9 years ago by
Attachment: | commercial-flagging-not-queued-workaround.patch0 added |
---|
comment:1 Changed 9 years ago by
Thanks to Bill Meek for the workaround. I've had to use it to patch programinfo.cpp every time that file gets updated on the trunk for the last 5 months. Isn't it about time to integrate it?
comment:3 Changed 9 years ago by
Resolution: | → Fixed |
---|---|
Status: | new → closed |
If this continues to be a problem for anyone then we need more information. The file size is not even stored in ProgramInfo? any more, they are saved to and read from RecordingFile? and the corresponding RecordedFile? table.
comment:5 Changed 9 years ago by
Resolution: | Fixed |
---|---|
Status: | closed → new |
Sorry, that doesn't solve the issue.
I added some LOG()s to demonstrate.
tv_rec.cpp:936 (FinishedRecording) - fsize>=1000 allows previewgen. fsize=0. ... tv_rec.cpp:989 (FinishedRecording) - fsize<1000 disables commflag/xcode. fsize=0.
Full log and LOG() patch attached.
comment:6 Changed 9 years ago by
To answer the questions on the mythtv channel, it's not the 0 byte file case. The recording plays back OK, manual commercial flagging works OK
mysql> SELECT recordedid FROM recorded WHERE title='Love It or List It'; | recordedid | | 2916 | mysql> SELECT r.filesize,rf.filesize FROM recorded AS r, recordedfile AS rf WHERE r.recordedid=2916 and rf.recordedid=2916; | filesize | filesize | | 883567100 | 883567100 |
comment:7 Changed 9 years ago by
Thanks for the follow up Bill. So it seems the cached RecordingInfo? used in TVRec isn't being updated ...
comment:10 Changed 9 years ago by
I added 2 more LOG()s and they return:
TVRecEvent recordinginfo.cpp:1628 (GetFilesize) - #12290 GetRecordingFile()->m_fileSize = 0 TVRecEvent recordinginfo.cpp:1637 (GetFilesize) - #12290 ProgramInfo::GetFilesize() = 0
And more LOG()s in RecordingFile::RecordingFile? and RecordingFile::Save:
This keeps incrementing during the recording:
recordingfile.cpp RecordingFile::Save() #12290, m_fileSize=405709264
Which then starts returning 0 (1268 times) after the recording finishes
Full log attached.
Changed 9 years ago by
Attachment: | mythbackend.20150422001210.13197.log added |
---|
Additional LOG()s added.
comment:11 Changed 9 years ago by
Can anything be done to further this along? This bug is quite annoying for those of us that don't build from source to manually patch.
If someone can point me where to start, I don't mind attempting a patch myself.
comment:12 Changed 8 years ago by
Milestone: | unknown → 0.28 |
---|
Changed 8 years ago by
Attachment: | 12290.patch added |
---|
comment:13 Changed 8 years ago by
POC patch, works fine, I just don't know how to treat the other 6 calls to FinishedRecording().
comment:14 Changed 8 years ago by
Bill, anything more required ? that last patch seems good enough to me.
comment:15 Changed 8 years ago by
Unfortunately, it isn't enough. When RingBufferChanged() (https://code.mythtv.org/cgit/mythtv/tree/mythtv/libs/libmythtv/tv_rec.cpp#n3358) is used, I've not been able to recover a size. I haven't seen the other 4 calls to FinishedRecording() used. Had to go back to the workaround in comment 1.
comment:16 Changed 8 years ago by
I can also report that the new patch is not a complete solution. Some recordings get flagged others don't. With the original patch all recordings that are set for auto flagging get flagged.
With neither patch, nothing gets auto flagged.
comment:21 Changed 8 years ago by
The "Hack fix" (31da15ad7) wasn't implemented correctly.
There are 2 instances of m_recording_file (in 2 separate RecInfos): the TVRec one is created at start of the recording and never updated, the Recorder one contains the updated filesize (and other data).
That patch intends to reload the stale instance but it is ineffective because the TVRec instance already exists. It needs to be forcibly reloaded.
I reverted the 4 patches on this ticket (2e98fb5832, 31da15ad7, cf4da125a78, 767370fe7e) and applied Bill's instrumentation to reproduce the before.log. Then applied the new patch to produce the after.log.
comment:22 Changed 8 years ago by
Milestone: | 29.0 → 0.28.1 |
---|---|
Owner: | set to stuartm |
Status: | new → accepted |
comment:23 Changed 7 years ago by
Milestone: | 0.28.1 → 0.28.2 |
---|
Moving remaining open 0.28.1 tickets to 0.28.2
comment:24 follow-up: 26 Changed 7 years ago by
Status: | accepted → infoneeded |
---|
Can anyone confirm whether the solution in comment 21 fixes the issue ?
comment:25 Changed 7 years ago by
Owner: | changed from stuartm to Roger Siddons |
---|
comment:26 Changed 7 years ago by
Replying to rsiddons:
Can anyone confirm whether the solution in comment 21 fixes the issue ?
Roger,
Only two tests, and flagging is queued (and works.) That's with the four reverts plus your fix. I should track it for a few days as others have reported inconsistent results with previous patches.
Looks like an edge case when the backend is restarted mid recording. The 2nd recording is flagged, the 1st isn't. I don't recall ever testing this originally and haven't tried with the old solution yet. Likely a new issue.
comment:27 Changed 7 years ago by
Thanks Bill,
Soak-test by all means but it would be nice if we could tidy this up for 28.2
The "old solution" was a work-around to the fact that a new implementation wasn't working properly.
I don't actually use the commflagger. I landed here because I had an issue with 0 filesizes elsewhere. Turns out it was unrelated...
However, isn't the job created by an end-of-recording event ? Should the backend check for interrupted recordings due for comm-flagging on startup ? User should start it manually IMO. And outside the scope of this ticket :)
comment:28 Changed 7 years ago by
Seems to be working great. Tested on v29-pre. Do you want to push the four reverts and fix or would you like me to?
comment:29 Changed 7 years ago by
Milestone: | 0.28.2 → 29.0 |
---|---|
Status: | infoneeded → assigned |
On review, it appears to no longer be an issue on 28/fixes: the work-around seems to be effective.
Whilst the fixes in comment:21 fix the specific issue on this ticket, the underlying changes that provoked it remain incomplete so it's not robust enough to be back-ported.
Targeting it for 29/master, once I've assessed what is still broken & the way forward.
Leaving this open to track it. Please report any observations of wrong filesizes.
comment:30 Changed 6 years ago by
Milestone: | 29.0 → 29.1 |
---|
comment:31 Changed 6 years ago by
Milestone: | 29.1 → 29.2 |
---|
comment:32 Changed 5 years ago by
Milestone: | 29.2 → 30.1 |
---|
comment:34 Changed 4 years ago by
Status: | assigned → infoneeded |
---|
Bill, is this still an issue, or can this be closed?
comment:35 Changed 4 years ago by
Resolution: | → Fixed |
---|---|
Status: | infoneeded → closed |
It's working. But I just undid a single conversion from Stuart Morgan's ProgramInfo? -> RecordedInfo? work. I see Roger kept it open to track the root cause.
Closing. A new ticket can be used to track the underlying issue if someone wants to go after it.
Workaround - tested on: v0.28-pre-2261-g53254f3