diff mbox series

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

Message ID 20200520200702.17742-1-cus@passwd.hu
State Accepted
Commit 6ec009f7e22d69502db83df49383b4a7c814ed7d
Headers show
Series [FFmpeg-devel] avformat/mpegts: use get_packet_size in mpegts_resync for determining raw_packet_size | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Marton Balint May 20, 2020, 8:07 p.m. UTC
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...
---
 libavformat/mpegts.c | 51 ++++++++++----------------------------------
 1 file changed, 11 insertions(+), 40 deletions(-)

Comments

Marton Balint May 26, 2020, 7:52 p.m. UTC | #1
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);
> +            if (ret < 0)
> +                return ret;
> +            new_packet_size = get_packet_size(s);
> +            if (new_packet_size > 0 && new_packet_size != ts->raw_packet_size) {
> +                av_log(ts->stream, AV_LOG_WARNING, "changing packet size to %d\n", new_packet_size);
> +                ts->raw_packet_size = new_packet_size;
> +            }
> +            avio_seek(pb, pos, SEEK_SET);
>             return 0;
>         }
>     }
> -- 
> 2.26.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lance Wang May 27, 2020, 1:23 a.m. UTC | #2
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. 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 (ret < 0)
> > +                return ret;
> > +            new_packet_size = get_packet_size(s);
> > +            if (new_packet_size > 0 && new_packet_size != ts->raw_packet_size) {
> > +                av_log(ts->stream, AV_LOG_WARNING, "changing packet size to %d\n", new_packet_size);
> > +                ts->raw_packet_size = new_packet_size;
> > +            }
> > +            avio_seek(pb, pos, SEEK_SET);
> >             return 0;
> >         }
> >     }
> > -- 
> > 2.26.1
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint May 27, 2020, 6:47 a.m. UTC | #3
On Wed, 27 May 2020, lance.lmwang@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.

Regards,
Marton
Lance Wang May 27, 2020, 10:16 a.m. UTC | #4
On Wed, May 27, 2020 at 08:47:54AM +0200, Marton Balint wrote:
> 
> 
> On Wed, 27 May 2020, lance.lmwang@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint May 27, 2020, 8:56 p.m. UTC | #5
On Wed, 27 May 2020, lance.lmwang@gmail.com wrote:

> On Wed, May 27, 2020 at 08:47:54AM +0200, Marton Balint wrote:
>> 
>> 
>> On Wed, 27 May 2020, lance.lmwang@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
diff mbox series

Patch

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);
+            if (ret < 0)
+                return ret;
+            new_packet_size = get_packet_size(s);
+            if (new_packet_size > 0 && new_packet_size != ts->raw_packet_size) {
+                av_log(ts->stream, AV_LOG_WARNING, "changing packet size to %d\n", new_packet_size);
+                ts->raw_packet_size = new_packet_size;
+            }
+            avio_seek(pb, pos, SEEK_SET);
             return 0;
         }
     }