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 locating first PCR in HD-PVR recordings #7988

Closed
wants to merge 1 commit into from

Conversation

bennettpeter
Copy link
Contributor

TV shows recorded with Hauppauge HD-PVR will play, but do not allow
seeking and do not show the length. This is because the TsDurationReader
cannot locate the first PCR in the first 112800 bytes. Increasing that
buffer to 122200 bytes solves the problem. To be on the safe side I am
setting it to double the original, or 225600 bytes. The search for last
PCR does not have this problem so I am leaving that at 122200 bytes.

TV shows recorded with Hauppauge HD-PVR will play, but do not allow
seeking and do not show the length. This is because the TsDurationReader
cannot locate the first PCR in the first 112800 bytes. Increasing that
buffer to 122200 bytes solves the problem. To be on the safe side I am
setting it to double the original, or 225600 bytes. The search for last
PCR does not have this problem so I am leaving that at 122200 bytes.
@ojw28
Copy link
Contributor

ojw28 commented Sep 25, 2020

Note: This is related to #5097.

@troyhough
Copy link

troyhough commented Sep 29, 2020

https://drive.google.com/file/d/1G1dOY5eu67kRu7h9IgNJglgqjsVgBB3M/view?usp=sharing

Here's a sample file that this change does not help with. Can the scope of this fix be expanded to help with a broader range of mpegts files?

Thanks in Advance!

@kim-vde
Copy link
Contributor

kim-vde commented Sep 30, 2020

The current method to compute the duration is already requiring a non-trivial amount of memory so we should avoid increasing it too much. We should come up with a method that requires less memory in the future.

@bennettpeter If increasing the buffer to 122200 bytes solves the issue, then I suggest we do that, to limit the memory consumption. I'll change the PR code myself.

@troyhough With the current way of computing the duration, supporting the stream you provided would require increasing the buffer from 112 800 bytes to more than 488 800 bytes. I will therefore not make the change as it would significantly increase the memory consumption for all TS files. Hopefully your stream will be better supported once we have optimized the duration computation.

@AquilesCanta
Copy link
Contributor

@kim-vde, should we consider letting the app choose a value for this through a TsExtractor parameter? Seems like that'd be a bit more scalable than updating on a per-demand basis.

I guess that if we were building a heuristic for this, that'd be based on the Transport Stream's bitrate, but that would require a considerably larger effort.

@kim-vde
Copy link
Contributor

kim-vde commented Sep 30, 2020

@AquilesCanta I was thinking of fetching the data by chunks instead of having one large buffer. But the parameter is a good idea. I'll do that instead.

@bennettpeter
Copy link
Contributor Author

@kim-vde A parameter would be good. I thought of that but it seems it would require changes in several classes to pass the parameter through, since the client code does not have access to the TSDurationReader class.

@troyhough
Copy link

@kim-vde Just so you know the test file I sent you was just a standard 30 minute HDHomerun recording from OTA, untouched. Probably the most common tuner on the market. Having said that I think it's pretty important to get seeking to work properly on something so common. Thanks for the consideration.

@bennettpeter
Copy link
Contributor Author

@kim-vde Can I update the pull request to allow the specification of the buffer size to be set in DefaultExtractorsFactory and TsExtractor? I will do that if it is likely to be accepted. Are there any special standards to be followed in this?

@kim-vde
Copy link
Contributor

kim-vde commented Oct 5, 2020

@bennettpeter I am actually busy doing that myself. It should be available in a few days.

@bennettpeter
Copy link
Contributor Author

I see that the sample from @troyhough requires SEARCH_BYTES_START at 1200 packets and SEARCH_BYTES_END at 2600 packets, where my original pullrequest was using 1200 and 600. So there will need to be the ability to modify both of those, or use one modifiable value for both.

@kim-vde
Copy link
Contributor

kim-vde commented Oct 6, 2020

Yes, I was going to use one variable for both.

kim-vde added a commit that referenced this pull request Oct 6, 2020
Context: Issue: #7988
PiperOrigin-RevId: 335608610
@kim-vde kim-vde closed this Oct 6, 2020
ojw28 pushed a commit that referenced this pull request Oct 21, 2020
Context: Issue: #7988
PiperOrigin-RevId: 335608610
@google google locked and limited conversation to collaborators Dec 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants