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

[lance.lmwang at gmail.com]

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.


> 
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".

-- 
Thanks,
Limin Wang