Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mythtranscode: Fix #244 by detecting write errors #164

Merged
merged 2 commits into from
Feb 12, 2021

Conversation

twitham1
Copy link
Contributor

@twitham1 twitham1 commented Mar 28, 2018

Fix for #244 which was originally https://code.mythtv.org/trac/ticket/12602

Explanation of the patch:

  • multiplex.h:
    the mx multiplex structure gets a new "int error", the error count.
    write_out_packs and finish_mpg now return int, the error count (so 0 = success)
    currently unused: callers test the error count instead
  • multiplex.c:
    if write() fails, increment error count
    log only first 10 failures so we don't flood syslog and cause it to drop messages
    if close() fails, increment error count
  • mpeg2fix.cpp:
    copy error count to static variable and pass it to pthread_exit
    capture error count in pthread_join and log/fail if non-zero

This all causes mythtv to keep the original recording if disk fills during transcode, instead of truncating the recording. Of course user will need to go free space larger than this recording for a successful transcode.

In mpeg2fix.cpp there is also a commented section where I tried to exit on first write failure. This just deadlocked the other thread. It might be possible to cancel the other thread if you wish to abort at first write error but I was unsure how to do this. As-is, we just continue to fail and return the failure count when the reader is done. I monitored with top and the size doesn't grow so I assume the data is just being dropped rather than filling memory, which is a good thing.

@twitham1 twitham1 changed the title Fix #12602 by detecting write errors (from fixes/0.27). mythtranscode: Fix #12602 by detecting write errors (from fixes/0.27). Mar 28, 2018
@twitham1 twitham1 changed the title mythtranscode: Fix #12602 by detecting write errors (from fixes/0.27). mythtranscode: Fix #12602 by detecting write errors Mar 28, 2018
@twitham1
Copy link
Contributor Author

See also #163 for original pull request against fixes/0.27.

@twitham1
Copy link
Contributor Author

twitham1 commented Sep 12, 2018

So we applied all fixes Beirdo recommended here and in #163 . Of course it still works at avoiding data loss at full disk during transcode, thereby resolving bug #12602. So not sure what more should be done here other than merge into master. Unless someone else wants to confirm the fix. How to do that is described in the bug intro at #244 or https://code.mythtv.org/trac/ticket/12602

@stuarta stuarta added this to Needs triage in Bug Triage Sep 1, 2020
@twitham1 twitham1 changed the title mythtranscode: Fix #12602 by detecting write errors mythtranscode: Fix #244 by detecting write errors Sep 30, 2020
@twitham1
Copy link
Contributor Author

twitham1 commented Feb 9, 2021

I reset my branch to current master and reduced the commit complexity down to the actual changes only. Detailed commit history is over in #163 if needed.

@twitham1
Copy link
Contributor Author

twitham1 commented Feb 9, 2021

Since @linuxdude42 most recently twiddled code in mythtranscode, I wonder if he would be willing to take a look at this?

@stuarta stuarta linked an issue Feb 10, 2021 that may be closed by this pull request
@linuxdude42
Copy link
Contributor

The changes look reasonable to me. I see only one minor flaw, which is that the "|| 0" in "return mx->error || 0;" is redundant.

@linuxdude42 linuxdude42 merged commit 351f43c into MythTV:master Feb 12, 2021
Bug Triage automation moved this from Needs triage to Closed Feb 12, 2021
@twitham1
Copy link
Contributor Author

Good point, that || 0 is a habit from my Perl coding, for upgrading an undef to zero. But in this C code the error is already initialized to 0 so this is not needed. You may remove my silly but harmless mistake if you like.

So glad to see this bug fixed, thank you sir!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Bug Triage
  
Closed
Development

Successfully merging this pull request may close these issues.

mythtranscode: full disk truncates recording
2 participants