diff mbox

[FFmpeg-devel] lavf/mpegts: improve read error handling

Message ID 20180823213823.36969-1-rodger.combs@gmail.com
State Withdrawn, archived
Headers show

Commit Message

Rodger Combs Aug. 23, 2018, 9:38 p.m. UTC 
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(-)

Comments

Jun Zhao Aug. 24, 2018, 3 a.m. UTC | #1 
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.
Hendrik Leppkes Aug. 24, 2018, 8:44 a.m. UTC | #2 
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
Rodger Combs Aug. 24, 2018, 4:21 p.m. UTC | #3 
> 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 mbox

Patch

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");