[FFmpeg-devel] [PATCH] avformat/mpegts: use get_packet_size in mpegts_resync for determining raw_packet_size

[Marton Balint]

On Wed, 27 May 2020, lance.lmwang at gmail.com wrote:

> On Wed, May 27, 2020 at 08:47:54AM +0200, Marton Balint wrote:
>> 
>> 
>> On Wed, 27 May 2020, lance.lmwang at gmail.com wrote:
>> 
>> > On Tue, May 26, 2020 at 09:52:45PM +0200, Marton Balint wrote:
>> > > 
>> > > 
>> > > On Wed, 20 May 2020, Marton Balint wrote:
>> > > 
>> > > > The old resync logic had some bugs, for example the packet size could stuck
>> > > > into 192 bytes, because pos47_full was not updated for every packet, and for
>> > > > unseekable inputs the resync logic simply skipped some 0x47 sync bytes,
>> > > > therefore the calculated distance between sync bytes was a multiple of 188
>> > > > bytes.
>> > > > > AVIO only buffers a single packet (for UDP/mpegts, that usually
>> > > means 1316
>> > > > bytes), so for every ten consecutive 188-byte MPEGTS packets there was always a
>> > > > seek failure, and that caused the old code to not find the 188 byte pattern
>> > > > across 10 consecutive packets.
>> > > > > This patch changes the custom logic to the one which is used
>> > > when probing to
>> > > > determine the packet size. This was already proposed as a FIXME for a long
>> > > > time...
>> > > 
>> > > Ping, will apply soon.
>> > > 
>> > > Regards,
>> > > Marton
>> > > 
>> > > > ---
>> > > > libavformat/mpegts.c | 51 ++++++++++----------------------------------
>> > > > 1 file changed, 11 insertions(+), 40 deletions(-)
>> > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> > > > index a065c61c40..c6fd3e1cef 100644
>> > > > --- a/libavformat/mpegts.c
>> > > > +++ b/libavformat/mpegts.c
>> > > > @@ -123,10 +123,6 @@ struct MpegTSContext {
>> > > >     /** raw packet size, including FEC if present */
>> > > >     int raw_packet_size;
>> > > > > -    int size_stat[3];
>> > > > -    int size_stat_count;
>> > > > -#define SIZE_STAT_THRESHOLD 10
>> > > > -
>> > > >     int64_t pos47_full;
>> > > > >     /** if true, all pids are analyzed to find streams */
>> > > > @@ -2840,41 +2836,6 @@ static int handle_packet(MpegTSContext *ts, const uint8_t *packet, int64_t pos)
>> > > >     return 0;
>> > > > }
>> > > > > -static void reanalyze(MpegTSContext *ts) {
>> > > > -    AVIOContext *pb = ts->stream->pb;
>> > > > -    int64_t pos = avio_tell(pb);
>> > > > -    if (pos < 0)
>> > > > -        return;
>> > > > -    pos -= ts->pos47_full;
>> > > > -    if (pos == TS_PACKET_SIZE) {
>> > > > -        ts->size_stat[0] ++;
>> > > > -    } else if (pos == TS_DVHS_PACKET_SIZE) {
>> > > > -        ts->size_stat[1] ++;
>> > > > -    } else if (pos == TS_FEC_PACKET_SIZE) {
>> > > > -        ts->size_stat[2] ++;
>> > > > -    }
>> > > > -
>> > > > -    ts->size_stat_count ++;
>> > > > -    if (ts->size_stat_count > SIZE_STAT_THRESHOLD) {
>> > > > -        int newsize = 0;
>> > > > -        if (ts->size_stat[0] > SIZE_STAT_THRESHOLD) {
>> > > > -            newsize = TS_PACKET_SIZE;
>> > > > -        } else if (ts->size_stat[1] > SIZE_STAT_THRESHOLD) {
>> > > > -            newsize = TS_DVHS_PACKET_SIZE;
>> > > > -        } else if (ts->size_stat[2] > SIZE_STAT_THRESHOLD) {
>> > > > -            newsize = TS_FEC_PACKET_SIZE;
>> > > > -        }
>> > > > -        if (newsize && newsize != ts->raw_packet_size) {
>> > > > -            av_log(ts->stream, AV_LOG_WARNING, "changing packet size to %d\n", newsize);
>> > > > -            ts->raw_packet_size = newsize;
>> > > > -        }
>> > > > -        ts->size_stat_count = 0;
>> > > > -        memset(ts->size_stat, 0, sizeof(ts->size_stat));
>> > > > -    }
>> > > > -}
>> > > > -
>> > > > -/* XXX: try to find a better synchro over several packets (use
>> > > > - * get_packet_size() ?) */
>> > > > static int mpegts_resync(AVFormatContext *s, int seekback, const uint8_t *current_packet)
>> > > > {
>> > > >     MpegTSContext *ts = s->priv_data;
>> > > > @@ -2896,8 +2857,18 @@ static int mpegts_resync(AVFormatContext *s, int seekback, const uint8_t *curren
>> > > >         if (avio_feof(pb))
>> > > >             return AVERROR_EOF;
>> > > >         if (c == 0x47) {
>> > > > +            int new_packet_size, ret;
>> > > >             avio_seek(pb, -1, SEEK_CUR);
>> > > > -            reanalyze(s->priv_data);
>> > > > +            pos = avio_tell(pb);
>> > > > +            ret = ffio_ensure_seekback(pb, PROBE_PACKET_MAX_BUF);
>> > 
>> > PROBE_PACKET_MAX_BUF is about 8K, maybe it'll cost too much for the detection with get_packet_size()
>> > during the runtime.
>> 
>> But the function uses avio_read_partial which only reads the amount which is
>> already in the input buffer, and only continues if the result is
>> inconclusive.
>> 
>> > How about to detect the next 3 sync byte with the probe size? it's used by vlc,
>> > refer to DetectPacketSize() in modules/demux/mpeg/ts.c
>> 
>> If you check the probing function, it uses PROBE_PACKET_MARGIN (5), which
>> means that 6 sync bytres are needed. Not much difference, so I'd rather keep
>> things as is.
>> 
>> Practically this only waits for at most one additional UDP packet, and even
>> that is not lost, because we ensure the buffer is seekable even for
>> non-streamed input.
>
> Thanks for the explanation, I'm fine with the patch.

Thanks, applied.

Regards,
Marton