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

[Rodger Combs]

> On Aug 24, 2018, at 03:44, Hendrik Leppkes  wrote:
> 
> On Fri, Aug 24, 2018 at 5:01 AM myp...@gmail.com  
> mailto:myp...@gmail.com>> wrote:
>> 
>> On Fri, Aug 24, 2018 at 5:38 AM Rodger Combs  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 
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel 
> 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Hendrik Leppkes]

On Fri, Aug 24, 2018 at 5:01 AM myp...@gmail.com  wrote:
>
> On Fri, Aug 24, 2018 at 5:38 AM Rodger Combs  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[myp...@gmail.com]

On Fri, Aug 24, 2018 at 5:38 AM Rodger Combs  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rodger Combs]

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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel