TsDemuxer seems to deterministically fail to demux final PES packet

[saurik]

TsDemuxer::demux calls TsParser::collectPes on every PES packet, which seems to only release the data for the packet (returning true) upon seeing the beginning of the next PES packet (with isPayloadStart)... but that means the last packet of every file I parse never gets dispatched :(. FWIW, I'd maybe expect this TODO to be relevant?

https://github.com/skullanbones/mpeg2ts/blob/d8f005556d6d311f1836004feb0ba73d4c0de1cd/libs/mpeg2ts/src/TsParser.cc#L326

[saurik]

FWIW, this is how I'd fix it, as someone who only understands how to program well but has almost no understanding of this specific file format (and so I have no clue why the size of those buffers is off by 6). The usage of .erase() instead of a default assignment is a lot cleaner here and I've made sure to avoid duplicated code paths.

diff --git a/libs/mpeg2ts/src/TsParser.cc b/libs/mpeg2ts/src/TsParser.cc
index b9a8e77..7a67c9a 100644
--- a/libs/mpeg2ts/src/TsParser.cc
+++ b/libs/mpeg2ts/src/TsParser.cc
@@ -282,29 +282,14 @@ bool TsParser::collectPes(const uint8_t* a_tsPacket, const TsPacketInfo& a_tsPac

     if (a_tsPacketInfo.isPayloadStart)
     {
-        // We have start. So if we have any cached data it's time to return it.
         if (!mPesPacket[pid].mPesBuffer.empty())
         {
-            if (mPesPacket[pid].PES_packet_length &&
-                mPesPacket[pid].mPesBuffer.size() < mPesPacket[pid].PES_packet_length)
-            {
-                std::cerr << "Not returning incomplete PES packet on pid " << pid << '\n';
-            }
-            else
-            {
-                a_pesPacket = mPesPacket[pid]; // TODO: must copy as we override it below.
-
-                mStatistics.buildPtsHistogram(pid, a_pesPacket.pts);
-                mStatistics.buildDtsHistogram(pid, a_pesPacket.dts);
-
-                ret = true;
-            }
+            std::cerr << "Not returning incomplete PES packet on pid " << pid << '\n';
+            mPesPacket.erase(pid);
         }

         mOrigins[pid] = origin;

-        // Create new PES
-        mPesPacket[pid] = PesPacket();
         pid = a_tsPacketInfo.pid;

         mPesPacket[pid].mPesBuffer.insert(mPesPacket[pid].mPesBuffer.end(),
@@ -323,7 +308,16 @@ bool TsParser::collectPes(const uint8_t* a_tsPacket, const TsPacketInfo& a_tsPac
         // Assemble packet
         mPesPacket[pid].mPesBuffer.insert(mPesPacket[pid].mPesBuffer.end(),
                                           &a_tsPacket[pointerOffset], &a_tsPacket[TS_PACKET_SIZE]);
-        // TODO: check if we have boud PES and return it if it is coplete
+
+        if (mPesPacket[pid].mPesBuffer.size() == mPesPacket[pid].PES_packet_length + 6) {
+            a_pesPacket = mPesPacket[pid];
+            mPesPacket.erase(pid);
+
+            mStatistics.buildPtsHistogram(pid, a_pesPacket.pts);
+            mStatistics.buildDtsHistogram(pid, a_pesPacket.dts);
+
+            ret = true;
+        }
     }

     return ret;

[kohnech]

Dear @saurik Thanks for your issue, I am looking into this and will release it with 0.7.0. You are right it was a left TODO, it was done intentionally for most user cases where you have an infinite UDP video stream and not file, but I think for file it makes sense to include the last PES packet.

Best regards, Kohnech

[kohnech]

fixed in #58