Bump go-astits and fix async KLV processing logic#291
Bump go-astits and fix async KLV processing logic#291kisasexypantera94 wants to merge 7 commits into
Conversation
|
I'll also add that I ultimately want to add support for ISO639 language codes to mpegts tracks, which was also fixed in the new go-astits release. So that's why I started this pull request haha |
|
Hello, here are a couple of suggestions:
Furthermore, this implementation computes the PTS of each incoming KLV unit by using the PTS of the nearest PES with PTS, this is not correct since the PTS should be the one of the last PES before the KLV. Some reasoning is needed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #291 +/- ##
==========================================
+ Coverage 84.54% 85.88% +1.34%
==========================================
Files 92 92
Lines 6515 5924 -591
==========================================
- Hits 5508 5088 -420
+ Misses 684 575 -109
+ Partials 323 261 -62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks! Sorry about the vendor folder, totally missed it, I'll remove it. Agreed on bounding pendingAsync: I’ll add a max queue size (and/or bytes) and drop with onDecodeError once exceeded to avoid unbounded RAM usage. Regarding timestamps: for async KLV received before the first timed PES, there is no “last PES before” available. If PCR is available in the adaptation field, we can use it as the initial timeline and timestamp early async KLV against the latest PCR observed, which matches the “last known time before KLV” semantics better. |
|
@aler9 Hi! I did not add PCR-based timing for async KLV yet. However, doing this correctly would require tracking a PCR snapshot per pending KLV, rather than relying on a single lastPCR, which adds some complexity to what is currently a relatively simple reader. If you think accurate PCR-based timing is required here, I can implement it. |
|
Unfortunately i can only confirm the timestamp issue i mentioned before: with this patch and even just with the new go-astits version, timestamp of async KLV units is computed wrongly. Multiple specifications tell that the PTS of an async KLV unit is the one of the previous PES with a PTS. The result of this patch is an incoherent PTS assignment. Let's suppose the decoder receives this packet sequence: The ideal behavior would be:
Contrarily, this patch and the new go-astits cause the following:
The correct implementation suggested by multiple sources is to parse the PES Header inside VIDEO PES B1, VIDEO PES D1, release it before the entire PES is returned and use it as lastPTS. This is tricky since we're using |
Store last valid PTS as soon as possible, by parsing PES headers in advance, then use this PTS as timestamp of KLV frames without PTS.
|
Replaced by #311 |
Store last valid PTS as soon as possible, by parsing PES headers in advance, then use this PTS as timestamp of KLV frames without PTS.
After go-astits PR #72, bounded PES packets (e.g. KLV metadata) can be emitted earlier than unbounded PES packets (e.g. video with PES_packet_length = 0).
This exposed a bug in the MPEG-TS reader where asynchronous KLV data was dropped if no PTS had been received yet.
This PR buffers async KLV data until the first PTS is available, then flushes it with the correct timestamp, instead of dropping it.