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

Failure to record HEVC/4K broadcast #620

Closed
kmdewaal opened this issue Aug 8, 2022 · 9 comments
Closed

Failure to record HEVC/4K broadcast #620

kmdewaal opened this issue Aug 8, 2022 · 9 comments
Labels
bug:hang-deadlock Hangs or deadlocks component:mythtv:recording Recording issues version:master Master Development Branch

Comments

@kmdewaal
Copy link
Contributor

kmdewaal commented Aug 8, 2022

MythTV master cannot record 4K broadcast from my cable signal. The same channel can be recorded OK with fixes/32.
Symptoms are zero-byte recordings and a backend that cannot be stopped with a single Ctrl-C.

The frontend gives the following messages:

2022-08-08 20:45:43.729934 I  TV::StartTV(): Entering main playback loop.
2022-08-08 20:45:58.166911 I  Player(0): Opening '/mnt/ava/livetv/131818_20220808184557.ts'
2022-08-08 20:46:04.750109 W  RingBuf(/mnt/ava/livetv/131818_20220808184557.ts): Taking too long to be allowed to read..
2022-08-08 20:46:11.267765 W  RingBuf(/mnt/ava/livetv/131818_20220808184557.ts): Taking too long to be allowed to read..
2022-08-08 20:46:17.786530 W  RingBuf(/mnt/ava/livetv/131818_20220808184557.ts): Taking too long to be allowed to read..
2022-08-08 20:46:24.303514 W  RingBuf(/mnt/ava/livetv/131818_20220808184557.ts): Taking too long to be allowed to read..
2022-08-08 20:46:28.168399 E  RingBuf(/mnt/ava/livetv/131818_20220808184557.ts): Took more than 30 seconds to be allowed to read, aborting.
2022-08-08 20:46:28.168403 W  RingBuf(/mnt/ava/livetv/131818_20220808184557.ts): Peek requested 2048 bytes but only have 0
2022-08-08 20:46:28.168409 E  Player(0): OpenFile(): Could not read first 2048 bytes of '/mnt/ava/livetv/131818_20220808184557.ts'
2022-08-08 20:46:28.168424 E  Player(0): Error Opening Jump Program File
2022-08-08 20:46:28.168425 E  PlayerUI: JumpToProgram failed.
2022-08-08 20:46:28.168428 E  PlayerUI: Unknown recorder error, exiting decoder
2022-08-08 20:46:28.169784 I  TV::HandleStateChange(): Attempting to change from WatchingLiveTV to None

The problem is introduced by the following commit:

[klaas@modu mythtv]$ git bisect bad
7b2ac1eeb593c0da760cf637a07536f874dce19b is the first bad commit
commit 7b2ac1eeb593c0da760cf637a07536f874dce19b
Author: Scott Theisen <scott.the.elm@gmail.com>
Date:   Mon Apr 4 15:45:19 2022 -0400

    replace internal ffmpeg headers with BitReader
    
    This should have no functional change.
    
    References https://github.com/MythTV/mythtv/issues/428
    
    The hack is no longer necessary since internal FFmpeg headers are no longer
    included by the parsers.
    
    avformatdecoder.cpp needed the public FFmpeg header "libavutil/intreadwrite.h".

 mythtv/libs/libmythtv/decoders/avformatdecoder.cpp |   1 +
 mythtv/libs/libmythtv/mpeg/AVCParser.cpp           | 206 +++++----
 mythtv/libs/libmythtv/mpeg/AVCParser.h             |   8 +-
 mythtv/libs/libmythtv/mpeg/H2645Parser.cpp         |  74 ++--
 mythtv/libs/libmythtv/mpeg/H2645Parser.h           |  20 +-
 mythtv/libs/libmythtv/mpeg/HEVCParser.cpp          | 480 ++++++++++-----------
 mythtv/libs/libmythtv/mpeg/HEVCParser.h            |  12 +-
 7 files changed, 382 insertions(+), 419 deletions(-)
[klaas@modu mythtv]$ 

@kmdewaal kmdewaal added version:master Master Development Branch bug:hang-deadlock Hangs or deadlocks component:mythtv:recording Recording issues labels Aug 8, 2022
@ulmus-scott
Copy link
Contributor

ulmus-scott commented Aug 8, 2022

I don't see how 7b2ac1e would introduce a problem; I'll have to look at the FFmpeg headers again to compare and see if there are any subtle differences.

Since this is a recording issue, a log from mythbackend -v record:debug would be helpful, espicially since HEVCParser has some log messages using VB_RECORD.


Edit: The FFmpeg headers are not very readable, but the only functional differences I saw were around invalid input data.

@kmdewaal
Copy link
Contributor Author

I do not understand what is the problem that is being solved by using your Bitreader functions instead of ffmpeg.
The ffmpeg functions are around since 2004 so all bugs will have been removed by now. Replacing it with something new can give regressions, as is the case here. Even if the replacement would be perfect then the only result is that MythTV still works and that there is additional software to maintain.
My recommendation is therefore to revert this commit 7b2ac1e.

@ulmus-scott
Copy link
Contributor

The point of that commit is to not use the internal FFmpeg headers get_bits.h and golomb.h and enable reverting the slight MythTV customizations to them regarding compiler warnings.

@BrettLMiller
Copy link

The change also removed this below const from the calculation of rbsp buffer:
AV_INPUT_BUFFER_PADDING_SIZE;
And removed the warning above it.

@ulmus-scott
Copy link
Contributor

The change also removed this below const from the calculation of rbsp buffer:
AV_INPUT_BUFFER_PADDING_SIZE;
And removed the warning above it.

That could be a significant difference.

Because MythTV didn't define CONFIG_SAFE_BITSTREAM_READER (since FFmpeg's config.h was not included) or UNCHECKED_BITSTREAM_READER, the UNCHECKED_BITSTREAM_READER was used, not performing boundary checks for past the end reads.

BitReader does perform these checks, and returns 0 if past the end instead of reading the 64 0xFF padding bytes as get_bits.h did.

@ulmus-scott
Copy link
Contributor

@kmdewaal Could you upload a log from mythbackend -v record:debug --logpath=SOME_PATH of making a recording before 7b2ac1e and with the current master where SOME_PATH is a valid path?

I would also like to clarify a few things about what specifically does/does not work:

  1. Do HEVC recordings from other channels work? Other 4K HEVC channels?
  2. Do HEVC recordings of a different resolution work?
  3. Do AVC (H.264) recordings still work?

@kmdewaal
Copy link
Contributor Author

As it is rather difficult to analyze this problem from a distance I did a bit of investigation.
This is the failing code, and a workaround, in HEVCparser.cpp around line 1880:

    uint8_t vps_max_layer_id = br.get_bits(6); // u(6)
#if 0   // KdW test  THIS fixes the bug...
    int vps_num_layer_sets_minus1 = br.get_ue_golomb(); // ue(v)
    for (int ui = 1; ui <= vps_num_layer_sets_minus1; ++ui)
#else
    uint vps_num_layer_sets_minus1 = br.get_ue_golomb(); // ue(v)
    for (i = 1; i <= vps_num_layer_sets_minus1; ++i)
#endif
    {
        for (int j = 0; j <= vps_max_layer_id; ++j)
        {
            br.get_bits(1); // layer_id_included_flag[i][j] u(1)
        }
    }

The problem happens when br.get_ue_golomb() returns a negative integer value, probably -1.
When copied into an uint this gives a very high value.... and then the for loop wants to read 4 billion bits. This then generates a timeout in mythfrontend and this keeps the backend thread DVBRead busy so that mythbackend cannot be terminated with a single Control-C.

It is of course possible, and probably a good idea, to protect the code in HEVCParser.cpp against negative values.

However, I do not think it is a good idea to make the BitReader behave different compared to the FFmpeg code it replaces, and then modify all the software that uses this.
My position on this is that, if we then really want to replace FFmpeg with code written by the team, that the code should be 100% compatible with the FFmpeg code that it replaces. We then do not need to change any other code and we can, if needed, go back to using the FFmpeg code.
To test this properly one does need a test harness which calls both the FFmpeg and the BitReader code, exercises the code with loads of data and then compares the results of each and every function call.

@ulmus-scott
Copy link
Contributor

That line was always incorrect:
https://github.com/MythTV/mythtv/blame/a0f653a2cce0416c1d7d4c1ba22dd1ffb37b315e/mythtv/libs/libmythtv/mpeg/HEVCParser.cpp#L1881
but get_ue_golomb() always returned a signed int https://github.com/MythTV/mythtv/blame/dcb64e970605d6b58a6d36da9ea83055fe40dd30/mythtv/external/FFmpeg/libavcodec/golomb.h#L55

So, in my opinion, your fix looks good to me, although I would have called the loop variable si for Signed Iterator.

I'm not sure why it worked with FFmpeg's version, though. For reference, AVERROR_INVALIDDATA = -1094995529, which is what FFmpeg would return on error.

From ITU-T-REC-H.265-202108-I!!PDF-E.pdf page 74: "The value of vps_num_layer_sets_minus1 shall be in the range of 0 to 1 023, inclusive." We could test less than 0, but, since it is only used by us once, it wouldn't matter as 1 is greater than 0.

Could you add these extra log statements and let me know what they print?

diff --git a/mythtv/libs/libmythtv/mpeg/HEVCParser.cpp b/mythtv/libs/libmythtv/mpeg/HEVCParser.cpp
index b9c1eb7123..5e1de4ac0f 100644
--- a/mythtv/libs/libmythtv/mpeg/HEVCParser.cpp
+++ b/mythtv/libs/libmythtv/mpeg/HEVCParser.cpp
@@ -1872,8 +1872,19 @@ bool HEVCParser::parseVPS(BitReader& br)
 #endif
 
     uint8_t vps_max_layer_id = br.get_bits(6); // u(6)
+    LOG(VB_GENERAL, LOG_ALERT, QString("br.show_bits(32) for get_ue_golomb() = %1")
+                                .arg(br.show_bits(32)));
+#if 0   // KdW test  THIS fixes the bug...
+    int vps_num_layer_sets_minus1 = br.get_ue_golomb(); // ue(v)
+    LOG(VB_GENERAL, LOG_ALERT, QString("vps_num_layer_sets_minus1 = %1")
+                                .arg(vps_num_layer_sets_minus1));
+    for (int ui = 1; ui <= vps_num_layer_sets_minus1; ++ui)
+#else
     uint vps_num_layer_sets_minus1 = br.get_ue_golomb(); // ue(v)
+    LOG(VB_GENERAL, LOG_ALERT, QString("vps_num_layer_sets_minus1 = %1")
+                                .arg(vps_num_layer_sets_minus1));
     for (i = 1; i <= vps_num_layer_sets_minus1; ++i)
+#endif
     {
         for (int j = 0; j <= vps_max_layer_id; ++j)
         {

kmdewaal added a commit that referenced this issue Aug 17, 2022
In parseVPS the value of vps_num_layer_sets_minus1 should always be zero or positive.
The value is returned by get_ue_golomb() but this is an int where a negative value
is used to signal an error status. Copying a negative value into an unsigned int and
using that as a loop counter limit results in an almost endless loop.
The negative value is observed incidentally with 4K HEVC recordings after the
change from FFmpeg to BitReader in commit 7b2ac1e.
However, also the FFmpeg version of get_ue_golomb() can return negative values
although this has never been observed.

This issue is now fixed by comparing int instead of unsigned int values.
Additionally, a warning message is given when the value returned by get_ue_golomb()
is negative when reading the vps_num_layer_sets_minus1 value.

This problem does happen only incidentally; it is possible that it is caused
by either invalid or unexpected HEVC stream content.
However, that is what is being broadcast and MythTV should be able to handle it.
It might also be possible that this issue is related to how close the reading
is to the head of the recording but this has not been further investigated.
Note that the get_ue_golomb() function is used in more places in HEVCParser.cpp
and in some of these places the code will fail horribly when a negative value
is returned. More fixes might be needed to make HEVCParser more resilient.

Refs #620
@kmdewaal
Copy link
Contributor Author

Today the problem cannot be reproduced anymore on the 4K HEVC cable channel that previously consistently failed to record. Whether that is due to the latest updates to master or to a change in the cable signal is difficult to say.
Protection against negative get_ue_golomb() value for vps_num_layer_sets_minus1 plus a warning message when this happens has been added to master; for the time being there is nothing more to be done. Therefore closing this ticket.

@kmdewaal kmdewaal mentioned this issue Sep 27, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:hang-deadlock Hangs or deadlocks component:mythtv:recording Recording issues version:master Master Development Branch
Projects
None yet
Development

No branches or pull requests

3 participants