Message ID | 20180823213823.36969-1-rodger.combs@gmail.com |
---|---|
State | Withdrawn, archived |
Headers | show |
On Fri, Aug 24, 2018 at 5:38 AM Rodger Combs <rodger.combs@gmail.com> wrote: > > We previously could fail to check errors entirely, or misinterpret read errors > as normal EOFs. > --- > libavformat/mpegts.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > index 37a6aa8bff..1350213d39 100644 > --- a/libavformat/mpegts.c > +++ b/libavformat/mpegts.c > @@ -2475,6 +2475,8 @@ static int mpegts_resync(AVFormatContext *s, int seekback, const uint8_t *curren > > for (i = 0; i < ts->resync_size; i++) { > c = avio_r8(pb); > + if (pb->error) > + return pb->error; > if (avio_feof(pb)) > return AVERROR_EOF; > if (c == 0x47) { > @@ -2498,8 +2500,13 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size, > > for (;;) { > len = ffio_read_indirect(pb, buf, TS_PACKET_SIZE, data); > - if (len != TS_PACKET_SIZE) > - return len < 0 ? len : AVERROR_EOF; > + if (len != TS_PACKET_SIZE) { > + if (len < 0) > + return len; > + if (pb->error) > + return pb->error; > + return AVERROR_EOF; > + } > /* check packet sync byte */ > if ((*data)[0] != 0x47) { > /* find a new packet start */ > @@ -2670,6 +2677,8 @@ static int mpegts_read_header(AVFormatContext *s) > /* read the first 8192 bytes to get packet size */ > pos = avio_tell(pb); > len = avio_read(pb, buf, sizeof(buf)); > + if (len < 0) > + return len; > ts->raw_packet_size = get_packet_size(buf, len); > if (ts->raw_packet_size <= 0) { > av_log(s, AV_LOG_WARNING, "Could not detect TS packet size, defaulting to non-FEC/DVHS\n"); > -- As I understand, only after avio_xxx return < 0 to check pb->error, now we coding like: len = avio_xxx(pb); if (len < 0) return len; if (pb->error) return pb->error; It's stranger way for me, consider Unix API read(2), we just check the error after -1 is returned ( http://man7.org/linux/man-pages/man2/read.2.html ) we usually catch the error / error number like: len = read(fd, buf, buf_size); if (len < 0) handle the error with error number.
On Fri, Aug 24, 2018 at 5:01 AM mypopy@gmail.com <mypopy@gmail.com> wrote: > > On Fri, Aug 24, 2018 at 5:38 AM Rodger Combs <rodger.combs@gmail.com> wrote: > > > > We previously could fail to check errors entirely, or misinterpret read > errors > > as normal EOFs. > > --- > > libavformat/mpegts.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c > > index 37a6aa8bff..1350213d39 100644 > > --- a/libavformat/mpegts.c > > +++ b/libavformat/mpegts.c > > @@ -2475,6 +2475,8 @@ static int mpegts_resync(AVFormatContext *s, int > seekback, const uint8_t *curren > > > > for (i = 0; i < ts->resync_size; i++) { > > c = avio_r8(pb); > > + if (pb->error) > > + return pb->error; > > if (avio_feof(pb)) > > return AVERROR_EOF; > > if (c == 0x47) { > > @@ -2498,8 +2500,13 @@ static int read_packet(AVFormatContext *s, uint8_t > *buf, int raw_packet_size, > > > > for (;;) { > > len = ffio_read_indirect(pb, buf, TS_PACKET_SIZE, data); > > - if (len != TS_PACKET_SIZE) > > - return len < 0 ? len : AVERROR_EOF; > > + if (len != TS_PACKET_SIZE) { > > + if (len < 0) > > + return len; > > + if (pb->error) > > + return pb->error; > > + return AVERROR_EOF; > > + } > > /* check packet sync byte */ > > if ((*data)[0] != 0x47) { > > /* find a new packet start */ > > @@ -2670,6 +2677,8 @@ static int mpegts_read_header(AVFormatContext *s) > > /* read the first 8192 bytes to get packet size */ > > pos = avio_tell(pb); > > len = avio_read(pb, buf, sizeof(buf)); > > + if (len < 0) > > + return len; > > ts->raw_packet_size = get_packet_size(buf, len); > > if (ts->raw_packet_size <= 0) { > > av_log(s, AV_LOG_WARNING, "Could not detect TS packet size, > defaulting to non-FEC/DVHS\n"); > > -- > As I understand, only after avio_xxx return < 0 to check pb->error, now we > coding like: > len = avio_xxx(pb); > if (len < 0) > return len; > if (pb->error) > return pb->error; > > It's stranger way for me, consider Unix API read(2), we just check the > error after -1 is returned > ( > http://man7.org/linux/man-pages/man2/read.2.html > ) > > we usually catch the error > / error > number like: > The reason for this is to be able to differentiate between EOF and read errors. In case of a read error, partial data may still be returned, and any short read is otherwise considered EOF before this patch. The alternative to this would be checking pb->eof_reached, which would do the same thing, just flip the logic on its head. - Hendrik
> On Aug 24, 2018, at 03:44, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > On Fri, Aug 24, 2018 at 5:01 AM mypopy@gmail.com <mailto:mypopy@gmail.com> <mypopy@gmail.com <mailto:mypopy@gmail.com>> wrote: >> >> On Fri, Aug 24, 2018 at 5:38 AM Rodger Combs <rodger.combs@gmail.com> wrote: >>> >>> We previously could fail to check errors entirely, or misinterpret read >> errors >>> as normal EOFs. >>> --- >>> libavformat/mpegts.c | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c >>> index 37a6aa8bff..1350213d39 100644 >>> --- a/libavformat/mpegts.c >>> +++ b/libavformat/mpegts.c >>> @@ -2475,6 +2475,8 @@ static int mpegts_resync(AVFormatContext *s, int >> seekback, const uint8_t *curren >>> >>> for (i = 0; i < ts->resync_size; i++) { >>> c = avio_r8(pb); >>> + if (pb->error) >>> + return pb->error; >>> if (avio_feof(pb)) >>> return AVERROR_EOF; >>> if (c == 0x47) { >>> @@ -2498,8 +2500,13 @@ static int read_packet(AVFormatContext *s, uint8_t >> *buf, int raw_packet_size, >>> >>> for (;;) { >>> len = ffio_read_indirect(pb, buf, TS_PACKET_SIZE, data); >>> - if (len != TS_PACKET_SIZE) >>> - return len < 0 ? len : AVERROR_EOF; >>> + if (len != TS_PACKET_SIZE) { >>> + if (len < 0) >>> + return len; >>> + if (pb->error) >>> + return pb->error; >>> + return AVERROR_EOF; >>> + } >>> /* check packet sync byte */ >>> if ((*data)[0] != 0x47) { >>> /* find a new packet start */ >>> @@ -2670,6 +2677,8 @@ static int mpegts_read_header(AVFormatContext *s) >>> /* read the first 8192 bytes to get packet size */ >>> pos = avio_tell(pb); >>> len = avio_read(pb, buf, sizeof(buf)); >>> + if (len < 0) >>> + return len; >>> ts->raw_packet_size = get_packet_size(buf, len); >>> if (ts->raw_packet_size <= 0) { >>> av_log(s, AV_LOG_WARNING, "Could not detect TS packet size, >> defaulting to non-FEC/DVHS\n"); >>> -- >> As I understand, only after avio_xxx return < 0 to check pb->error, now we >> coding like: >> len = avio_xxx(pb); >> if (len < 0) >> return len; >> if (pb->error) >> return pb->error; >> >> It's stranger way for me, consider Unix API read(2), we just check the >> error after -1 is returned >> ( >> http://man7.org/linux/man-pages/man2/read.2.html >> ) >> >> we usually catch the error >> / error >> number like: >> > > The reason for this is to be able to differentiate between EOF and > read errors. In case of a read error, partial data may still be > returned, and any short read is otherwise considered EOF before this > patch. > The alternative to this would be checking pb->eof_reached, which would > do the same thing, just flip the logic on its head. I'd actually sort of prefer that, since it'd let me call avio_feof() instead of doing a direct member access, but it turns out that eof_reached is set to 1 in error cases as well. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c index 37a6aa8bff..1350213d39 100644 --- a/libavformat/mpegts.c +++ b/libavformat/mpegts.c @@ -2475,6 +2475,8 @@ static int mpegts_resync(AVFormatContext *s, int seekback, const uint8_t *curren for (i = 0; i < ts->resync_size; i++) { c = avio_r8(pb); + if (pb->error) + return pb->error; if (avio_feof(pb)) return AVERROR_EOF; if (c == 0x47) { @@ -2498,8 +2500,13 @@ static int read_packet(AVFormatContext *s, uint8_t *buf, int raw_packet_size, for (;;) { len = ffio_read_indirect(pb, buf, TS_PACKET_SIZE, data); - if (len != TS_PACKET_SIZE) - return len < 0 ? len : AVERROR_EOF; + if (len != TS_PACKET_SIZE) { + if (len < 0) + return len; + if (pb->error) + return pb->error; + return AVERROR_EOF; + } /* check packet sync byte */ if ((*data)[0] != 0x47) { /* find a new packet start */ @@ -2670,6 +2677,8 @@ static int mpegts_read_header(AVFormatContext *s) /* read the first 8192 bytes to get packet size */ pos = avio_tell(pb); len = avio_read(pb, buf, sizeof(buf)); + if (len < 0) + return len; ts->raw_packet_size = get_packet_size(buf, len); if (ts->raw_packet_size <= 0) { av_log(s, AV_LOG_WARNING, "Could not detect TS packet size, defaulting to non-FEC/DVHS\n");