demux: mpegts: handle "empty" p_pkt in PushPESBlock()

Hi!

When a discontinuity is indicated, my bluray subtitle pid reads an empty PES packet (p_pkt->i_buffer==0). I think that such packets are not supposed to be chained in PushPESBlock(), since they change the expected relation between gather.data (not null) and gather.i_data_size (0) on subsequent subtitle packets processing.

The noticeable behaviour observed when a discontinuity is crossed is that subtitles packets are processed only after the next packet has been pushed. This causes subtitles to remain displayed on screen until the next subtitle replaces it: the last subtitle packet usually is the "end of display set segment", and it can remain gathered and not pushed for a potentially long time.

Possible fix

Added attachment:

• 1000-demux-ts-fix-pes-gathering-with-empty-packet.patch

Replying to [ticket:23654 Fabrice Bellet]:

Hi!

That does not seem correct. If you receive another packet, then you have unit_start and the previous packet is output.

The whole thing is not clear. Need to see the packet bytes.

vlc log with debug information added in function PushPESBlock(), restricted to subtitle PES

Added attachment:

• vlc.log

OK, the twist happens at line 663 in the log file, when PushPESBlock() is called with a pkt having i_buffer=0.

Behaviour before that line is correct: packet passed to ParsePESDataChain() is the last packet passed to PushPESBlock() with unit_start=1.

Starting from this line 663, gather.i_data_size remains to the value "0", and due to this value, it introduces an difference of "one" packet between the packet passed to PushPESBlock() with start_unit=1 and the packet passed to ParsePESDataChain(). There's always one packet "late" from this point.

It could be unnoticed for most streams, but it becomes visible for subtitles, where a packet "removing" a subtitle element on screen (i_buffer=17, and p_buffer contains "80 00 00" at position 14) can remain unparsed for a long time, until the next packet arrives, because subtitles packets don't have a fixed rate.

The root cause of this new behaviour is in GatherPESData() where a new gather.i_data_size value is set from p_pkt when gather.p_data==null. But this condition is not met after the empty packet: PushPESBlock() exits with gather.p_data not null, and gather.i_data_size==0.

You can't tell from this point if the packet has zero data payload or is truncated to zero and expects to be merged. I think the problem is more global, ie, when you switch to fixed size payload from variable size.

Oh I see, this packet is injected in the stream by the bluray module in notifyDiscontinuityToParser() with DONOTIFY(pg).

Replying to [comment:4 Fabrice Bellet]:

Oh I see, this packet is injected in the stream by the bluray module in notifyDiscontinuityToParser() with DONOTIFY(pg).

Mmm, that's strange. The PES payload on discontinuity only exists for video

So seems there's 2 issues here:

• The 0 payload being considered as 0 PES size (variable, wait next sync/packet).
• The recovery when in variable size, to a fixed size in the same packet. I'm writing tests

There's few other issues, but as expected, the problem exists with variable payload followed by fixed payload which become variable.

changing last part of Gather code seems fixing the general case for now

            else /* if( p_pes->gather.i_data_size == 0 ) // see next packet */
            {
                if( likely(b_aligned_ts_payload) && b_unit_start )
                {
                    b_ret |= ts_pes_Push( cb, p_pes, NULL, true );
                    /* now points to PES header */
                    if( p_pkt->i_buffer >= 6 )
                    {
                        p_pes->gather.i_data_size = GetWBE(&p_pkt->p_buffer[4]);
                        if( p_pes->gather.i_data_size > 0 )
                            p_pes->gather.i_data_size += 6;
                    }
                }
                /* Append or finish current/start new PES depending on unit_start */
                b_ret |= ts_pes_Push( cb, p_pes, p_pkt, b_unit_start );
                p_pkt = NULL;
            }

This modification works for me (tested with the vlc-3.0 branch). Thank you for that fix!

6efb4c78

added Component::Demuxers Status::fixed labels

closed