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

TS Extractor: Fix problem with corrupt last PCR in recordings #9100

Closed

Conversation

bennettpeter
Copy link
Contributor

Some recorded TV shows are picking up a corrupt value for last PCR in the
file. The result is that the recording only plays for a few seconds. In
the example provided, the duration came out as a large negative number,
and the file played for a few seconds and then reported playback as complete.

The logic in TsDurationReader searches backwards in the file looking
for a valid PCR. Either it is picking up a corrupt value or it is
mis-interpreting other data as if it was a PCR.

The fix searches for two PCR values and performs a sanity check to
ensure they are within valid range before accepting the value. If they
are not valid, it continues searching until it finds valid values.

Some recorded TV shows are picking up a corrupt value for last PCR in the
file. The result is that the recording only plays for a few seconds. In
the example provided, the duration came out as a large negative number,
and the file played for a few seconds and then reported playback as complete.

The logic in TsDurationReader searches backwards in the file looking
for a valid PCR. Either it is picking up a corrupt value or it is
mis-interpreting other data as if it was a PCR.

The fix searches for two PCR values and performs a sanity check to
ensure they are within valid range before accepting the value. If they
are not valid, it continues searching until it finds valid values.
@google-cla google-cla bot added the cla: yes label Jun 22, 2021
@bennettpeter
Copy link
Contributor Author

@kim-vde
Copy link
Contributor

kim-vde commented Jul 12, 2021

Hi. Sorry that I took so long to check this.

What bothers me a bit with this fix is that it is not backward compatible. For the same value of timestampSearchBytes, readLastPcrValueFromBuffer now needs to find at least 2 PCRs instead of 1 to compute the duration. We could double the size of the buffer in which to search, but then we would slow down the preparation time and store a buffer that is twice larger, even for valid streams.

The result is that the recording only plays for a few seconds.

That is not what I observe in our demo app. With the stream you provided, the duration is indeed incorrect (it is displayed as -7:10:26), but the video plays until the end (during 02:01).

Do you observe the same things as me with the demo app? In other words, is this fix only about displaying the correct duration?

@bennettpeter
Copy link
Contributor Author

Do you observe the same things as me with the demo app? In other words, is this fix only about displaying the correct duration?

My code attempted to change audio channel and that caused playback to end. Also skipping back or forward may cause playback to end. The original error reporter said that they could not skip forward or back.

@AquilesCanta
Copy link
Contributor

We have found a bug in the duration reader. Thanks for sharing the problematic media with us. Once we fix internally and push to github, your stream will work as expected.

Explanation: The duration reader doesn't synchronize to the transport stream. It just checks for they existence of a sync byte, which can happen at any point in the stream. So the fix is only assuming a packet starts if we can find 5 consecutive 5 bytes, as per the transport stream specification. Please see the commit that will appear below for more details.

@AquilesCanta
Copy link
Contributor

Also, I'll use this issue to track the problem, so that we don't need to file a fresh one.

icbaker pushed a commit that referenced this pull request Jul 16, 2021
#minor-release
Issue: #9100
PiperOrigin-RevId: 384962258
icbaker pushed a commit that referenced this pull request Jul 21, 2021
#minor-release
Issue: #9100
PiperOrigin-RevId: 384962258
@bennettpeter
Copy link
Contributor Author

Thank you for fixing this. My pull request was more a workaround than a fix, since I do not understand the details of the TS stream.

@AquilesCanta
Copy link
Contributor

AquilesCanta commented Jul 21, 2021

@bennettpeter have you tested the fix by any chance? EDIT: Thanks for your contribution, though! It's still a valid approach.

@bennettpeter
Copy link
Contributor Author

I have not tested it yet. I will integrate the new version into my app in the next few days, test it and post the result here. Should the pull request be kept open unitil I have tested and confirmed?

@AquilesCanta
Copy link
Contributor

No, that's ok. We usually close after the fix is in place. You can file a fresh issue if you run into any further problems.

@bennettpeter
Copy link
Contributor Author

I upgraded my app to the new release 2.14.2 , which includes this fix. I have tested it and it is working perfectly. Thank you!

@google google locked and limited conversation to collaborators Sep 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants