[FFmpeg-devel] [PATCH] avformat/matroskadec: return AVERROR(EIO) rather than AVERROR_EOF on parse error

2016-07-22 Thread Sophia Wang
Signed-off-by: Sophia Wang <s...@google.com>
---
 libavformat/matroskadec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index f3d701f..c536605 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -3323,7 +3323,7 @@ static int matroska_read_packet(AVFormatContext *s, 
AVPacket *pkt)
 while (matroska_deliver_packet(matroska, pkt)) {
 int64_t pos = avio_tell(matroska->ctx->pb);
 if (matroska->done)
-return AVERROR_EOF;
+return avio_feof(s->pb) ? AVERROR_EOF : AVERROR(EIO);
 if (matroska_parse_cluster(matroska) < 0)
 matroska_resync(matroska, pos);
 }
-- 
2.8.0.rc3.226.g39d4020

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


[FFmpeg-devel] [PATCH] avformat/matroskadec: retain error codes in matroska_resync() and matroska_read_packet()

2016-08-10 Thread Sophia Wang
Signed-off-by: Sophia Wang <s...@google.com>
---
 libavformat/matroskadec.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index d07a092..8c809ad 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -738,13 +738,16 @@ static int matroska_read_close(AVFormatContext *s);
 static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)
 {
 AVIOContext *pb = matroska->ctx->pb;
+int64_t ret;
 uint32_t id;
 matroska->current_id = 0;
 matroska->num_levels = 0;
 
 /* seek to next position to resync from */
-if (avio_seek(pb, last_pos + 1, SEEK_SET) < 0)
-goto eof;
+if ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) {
+matroska->done = 1;
+return ret;
+}
 
 id = avio_rb32(pb);
 
@@ -760,7 +763,6 @@ static int matroska_resync(MatroskaDemuxContext *matroska, 
int64_t last_pos)
 id = (id << 8) | avio_r8(pb);
 }
 
-eof:
 matroska->done = 1;
 return AVERROR_EOF;
 }
@@ -3322,13 +3324,16 @@ static int matroska_parse_cluster(MatroskaDemuxContext 
*matroska)
 static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt)
 {
 MatroskaDemuxContext *matroska = s->priv_data;
+int ret;
 
 while (matroska_deliver_packet(matroska, pkt)) {
 int64_t pos = avio_tell(matroska->ctx->pb);
 if (matroska->done)
 return AVERROR_EOF;
-if (matroska_parse_cluster(matroska) < 0)
-matroska_resync(matroska, pos);
+if (matroska_parse_cluster(matroska) < 0) {
+if ((ret = matroska_resync(matroska, pos)) < 0)
+return ret;
+}
 }
 
 return 0;
-- 
2.8.0.rc3.226.g39d4020

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


Re: [FFmpeg-devel] [PATCH] avformat/matroskadec: retain error codes in matroska_resync() and matroska_read_packet()

2016-08-10 Thread Sophia Wang
Apologies, I didn't realize that matroska->done was only checked in one
place. But Nicolas raises a good point about the subtle change in behavior.
I did not intend to change the overall behavior of the code for clients, so
I'll modify the patch to restore the matroska->done check.

On Tue, Aug 9, 2016 at 5:58 AM, Nicolas George  wrote:

> Le tridi 23 thermidor, an CCXXIV, Michael Niedermayer a écrit :
> > prior to this the "done" field was used to stop further work when
> > EOF was reached, now this is removed.
> > The patches commit message doesnt say anything why all reads for the
> > done field are removed or what if any performance impact that has
> >
> > the patch even adds code setting the done variable even though all
> > reads of the varable are removed
> > This doesnt look right
>
> If I read the code correctly, on the first round the result will be roughly
> the same, with just a more accurate error code. The difference about the
> done flag will appear if the application tries again to read a packet after
> an error: without the check on done, it will try to resync again, which may
> be a good thing in some cases.
>
> But of course, changing it inadvertently is not a good thing, and neither
> keeping dead code for setting an unused flag.
>
> Regards,
>
> --
>   Nicolas George
>
> ___
> 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] avformat/matroskadec: return AVERROR(EIO) rather than AVERROR_EOF on parse error

2016-08-01 Thread Sophia Wang
Thanks for the quick response. matroska_resync() is currently written to
only return AVERROR_EOF, so using that error code directly wouldn't be any
more informative. But your suggestion would work if matroska_resync() kept
the error code returned by avio_seek(). Would such a change warrant
splitting into a separate patch?

On Sun, Jul 31, 2016 at 10:26 AM, Nicolas George <geo...@nsup.org> wrote:

> Le quartidi 14 thermidor, an CCXXIV, Sophia Wang a écrit :
> > Since matroska->done is only set to 1 in matroska_resync(), the choice
> > of error is made by checking the return value of matroska_resync()
> > rather than checking matroska->done directly on the next
> > while-iteration.
>
> This is not what your code do:
>
> > +if (matroska_resync(matroska, pos) < 0)
> > +return avio_feof(s->pb) ? AVERROR_EOF : AVERROR(EIO);
>
> It checks the return value of matroska_resync() and then invents a
> completely unrelated error code.
>
> What it should do is use the error code from matroska_resync(). As simple
> as
> that.
>
> Regards,
>
> --
>   Nicolas George
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


[FFmpeg-devel] [PATCH] avformat/matroskadec: retain error codes in matroska_resync() and matroska_read_packet()

2016-08-02 Thread Sophia Wang
Signed-off-by: Sophia Wang <s...@google.com>
---
 libavformat/matroskadec.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index d07a092..f9693ca 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -738,13 +738,16 @@ static int matroska_read_close(AVFormatContext *s);
 static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)
 {
 AVIOContext *pb = matroska->ctx->pb;
+int64_t ret;
 uint32_t id;
 matroska->current_id = 0;
 matroska->num_levels = 0;
 
 /* seek to next position to resync from */
-if (avio_seek(pb, last_pos + 1, SEEK_SET) < 0)
-goto eof;
+if ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) {
+matroska->done = 1;
+return ret;
+}
 
 id = avio_rb32(pb);
 
@@ -760,7 +763,6 @@ static int matroska_resync(MatroskaDemuxContext *matroska, 
int64_t last_pos)
 id = (id << 8) | avio_r8(pb);
 }
 
-eof:
 matroska->done = 1;
 return AVERROR_EOF;
 }
@@ -3322,13 +3324,14 @@ static int matroska_parse_cluster(MatroskaDemuxContext 
*matroska)
 static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt)
 {
 MatroskaDemuxContext *matroska = s->priv_data;
+int ret;
 
 while (matroska_deliver_packet(matroska, pkt)) {
 int64_t pos = avio_tell(matroska->ctx->pb);
-if (matroska->done)
-return AVERROR_EOF;
-if (matroska_parse_cluster(matroska) < 0)
-matroska_resync(matroska, pos);
+if (matroska_parse_cluster(matroska) < 0) {
+if ((ret = matroska_resync(matroska, pos)) < 0)
+return ret;
+}
 }
 
 return 0;
-- 
2.8.0.rc3.226.g39d4020

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


Re: [FFmpeg-devel] [PATCH] avformat/matroskadec: retain error codes in matroska_resync() and matroska_read_packet()

2016-08-15 Thread Sophia Wang
Of the 3 functions that directly add to matroska->packets, 2 of them will
always return 0 afterwards, and the third (matroska_parse_rm_audio) will
only return something other than 0 in the case of AVERROR(ENOMEM) or
AVERROR(EINVAL). All 3 functions are called from matroska_parse_block,
which immediately returns the result if there is no error. If an error does
occur, then the error code may eventually be returned, or it may be
overwritten by the return value of any of the 3 packet-adding functions.

matroska_parse_block is called from matroska_parse_cluster and its helper
matroska_parse_cluster_incremental, both of which simply return the result
(though it maybe overwritten by another invocation in
matroska_parse_cluster). So in any case, if packets are added, the only
sources of failure are ENOMEM and EINVAL, and I'm not sure if it makes
sense to write code to work around those.

On Wed, Aug 10, 2016 at 4:51 PM, Michael Niedermayer <mich...@niedermayer.cc
> wrote:

> On Wed, Aug 10, 2016 at 10:24:08AM -0700, Sophia Wang wrote:
> > Signed-off-by: Sophia Wang <s...@google.com>
> > ---
> >  libavformat/matroskadec.c | 15 ++-
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > index d07a092..8c809ad 100644
> > --- a/libavformat/matroskadec.c
> > +++ b/libavformat/matroskadec.c
> > @@ -738,13 +738,16 @@ static int matroska_read_close(AVFormatContext
> *s);
> >  static int matroska_resync(MatroskaDemuxContext *matroska, int64_t
> last_pos)
> >  {
> >  AVIOContext *pb = matroska->ctx->pb;
> > +int64_t ret;
> >  uint32_t id;
> >  matroska->current_id = 0;
> >  matroska->num_levels = 0;
> >
> >  /* seek to next position to resync from */
> > -if (avio_seek(pb, last_pos + 1, SEEK_SET) < 0)
> > -goto eof;
> > +if ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) {
> > +matroska->done = 1;
> > +return ret;
> > +}
> >
> >  id = avio_rb32(pb);
> >
> > @@ -760,7 +763,6 @@ static int matroska_resync(MatroskaDemuxContext
> *matroska, int64_t last_pos)
> >  id = (id << 8) | avio_r8(pb);
> >  }
> >
> > -eof:
> >  matroska->done = 1;
> >  return AVERROR_EOF;
> >  }
>
> > @@ -3322,13 +3324,16 @@ static int 
> > matroska_parse_cluster(MatroskaDemuxContext
> *matroska)
> >  static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt)
> >  {
> >  MatroskaDemuxContext *matroska = s->priv_data;
> > +int ret;
> >
> >  while (matroska_deliver_packet(matroska, pkt)) {
> >  int64_t pos = avio_tell(matroska->ctx->pb);
> >  if (matroska->done)
> >  return AVERROR_EOF;
> > -if (matroska_parse_cluster(matroska) < 0)
> > -matroska_resync(matroska, pos);
> > +if (matroska_parse_cluster(matroska) < 0) {
> > +if ((ret = matroska_resync(matroska, pos)) < 0)
> > +return ret;
> > +}
> >  }
>
> is it possible that matroska_parse_cluster() adds packets to
> matroska->packets and then fails?
> if so and if matroska_resync subsequently fails too
> the previous code would have returned the packet in
> matroska_deliver_packet() the new code would not i think unless
> i miss something
> (the application would likley not call matroska_read_packet() again
> after it signaled EOF, and might or might not after an error even
> though there would the still be buffered packets prior to the error
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> No great genius has ever existed without some touch of madness. --
> Aristotle
>
> ___
> 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] avformat/matroskadec: retain error codes in matroska_resync() and matroska_read_packet()

2016-09-22 Thread Sophia Wang
I think you're right. Seems like the best solution is to maintain the
number of calls to matroska_deliver_packet(); patch modification on the way.

On Wed, Aug 31, 2016 at 6:30 PM, Michael Niedermayer <mich...@niedermayer.cc
> wrote:

>
> On Mon, Aug 15, 2016 at 10:27:47AM -0700, Sophia Wang wrote:
> > Of the 3 functions that directly add to matroska->packets, 2 of them will
> > always return 0 afterwards, and the third (matroska_parse_rm_audio) will
> > only return something other than 0 in the case of AVERROR(ENOMEM) or
> > AVERROR(EINVAL).
>
> but they can add multiple packets, so even if they return 0 there
> could be packets in the buffer after one is consumed
>
>
> > All 3 functions are called from matroska_parse_block,
> > which immediately returns the result if there is no error.
>
> did you mean "if there is AN error" ?
>
>
> > If an error does
> > occur, then the error code may eventually be returned, or it may be
> > overwritten by the return value of any of the 3 packet-adding functions.
> >
> > matroska_parse_block is called from matroska_parse_cluster and its helper
> > matroska_parse_cluster_incremental, both of which simply return the
> result
> > (though it maybe overwritten by another invocation in
> > matroska_parse_cluster). So in any case, if packets are added, the only
> > sources of failure are ENOMEM and EINVAL, and I'm not sure if it makes
> > sense to write code to work around those.
>
> you add code in matroska_resync() and its caller to immedeatly
> break out and return failure if the seek fails.
> At this point there can still be packets in the buffers unless i
> miss something, should the error not be returned after the packets
> that where read before the error ?
>
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> No great genius has ever existed without some touch of madness. --
> Aristotle
>
> ___
> 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


[FFmpeg-devel] [PATCH] avformat/matroskadec: retain error codes in matroska_resync() and matroska_read_packet()

2016-09-22 Thread Sophia Wang
Signed-off-by: Sophia Wang <s...@google.com>
---
 libavformat/matroskadec.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 77b8a5d..936690d 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -738,13 +738,16 @@ static int matroska_read_close(AVFormatContext *s);
 static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)
 {
 AVIOContext *pb = matroska->ctx->pb;
+int64_t ret;
 uint32_t id;
 matroska->current_id = 0;
 matroska->num_levels = 0;
 
 /* seek to next position to resync from */
-if (avio_seek(pb, last_pos + 1, SEEK_SET) < 0)
-goto eof;
+if ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) {
+matroska->done = 1;
+return ret;
+}
 
 id = avio_rb32(pb);
 
@@ -760,7 +763,6 @@ static int matroska_resync(MatroskaDemuxContext *matroska, 
int64_t last_pos)
 id = (id << 8) | avio_r8(pb);
 }
 
-eof:
 matroska->done = 1;
 return AVERROR_EOF;
 }
@@ -3317,13 +3319,14 @@ static int matroska_parse_cluster(MatroskaDemuxContext 
*matroska)
 static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt)
 {
 MatroskaDemuxContext *matroska = s->priv_data;
+int ret = 0;
 
 while (matroska_deliver_packet(matroska, pkt)) {
 int64_t pos = avio_tell(matroska->ctx->pb);
 if (matroska->done)
-return AVERROR_EOF;
+return (ret < 0) ? ret : AVERROR_EOF;
 if (matroska_parse_cluster(matroska) < 0)
-matroska_resync(matroska, pos);
+ret = matroska_resync(matroska, pos);
 }
 
 return 0;
-- 
2.8.0.rc3.226.g39d4020

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


[FFmpeg-devel] [PATCH] avformat/matroskadec: retain error codes in matroska_resync() and matroska_read_packet()

2016-09-27 Thread Sophia Wang
Signed-off-by: Sophia Wang <s...@google.com>
---
 libavformat/matroskadec.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 77b8a5d..7ee1c7a 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -738,13 +738,16 @@ static int matroska_read_close(AVFormatContext *s);
 static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)
 {
 AVIOContext *pb = matroska->ctx->pb;
+int64_t ret;
 uint32_t id;
 matroska->current_id = 0;
 matroska->num_levels = 0;
 
 /* seek to next position to resync from */
-if (avio_seek(pb, last_pos + 1, SEEK_SET) < 0)
-goto eof;
+if ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) {
+matroska->done = 1;
+return ret;
+}
 
 id = avio_rb32(pb);
 
@@ -760,7 +763,6 @@ static int matroska_resync(MatroskaDemuxContext *matroska, 
int64_t last_pos)
 id = (id << 8) | avio_r8(pb);
 }
 
-eof:
 matroska->done = 1;
 return AVERROR_EOF;
 }
@@ -3317,16 +3319,17 @@ static int matroska_parse_cluster(MatroskaDemuxContext 
*matroska)
 static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt)
 {
 MatroskaDemuxContext *matroska = s->priv_data;
+int ret = 0;
 
 while (matroska_deliver_packet(matroska, pkt)) {
 int64_t pos = avio_tell(matroska->ctx->pb);
 if (matroska->done)
-return AVERROR_EOF;
+return (ret < 0) ? ret : AVERROR_EOF;
 if (matroska_parse_cluster(matroska) < 0)
-matroska_resync(matroska, pos);
+ret = matroska_resync(matroska, pos);
 }
 
-return 0;
+return ret;
 }
 
 static int matroska_read_seek(AVFormatContext *s, int stream_index,
-- 
2.8.0.rc3.226.g39d4020

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


Re: [FFmpeg-devel] [PATCH] avformat/matroskadec: retain error codes in matroska_resync() and matroska_read_packet()

2016-09-27 Thread Sophia Wang
On Fri, Sep 23, 2016 at 1:40 AM, Benoit Fouet <benoit.fo...@free.fr> wrote:

> Hi,
>
>
> On 22/09/2016 23:03, Sophia Wang wrote:
>
>> Signed-off-by: Sophia Wang <s...@google.com>
>> ---
>>   libavformat/matroskadec.c | 13 -
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 77b8a5d..936690d 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -738,13 +738,16 @@ static int matroska_read_close(AVFormatContext *s);
>>   static int matroska_resync(MatroskaDemuxContext *matroska, int64_t
>> last_pos)
>>   {
>>   AVIOContext *pb = matroska->ctx->pb;
>> +int64_t ret;
>>   uint32_t id;
>>   matroska->current_id = 0;
>>   matroska->num_levels = 0;
>> /* seek to next position to resync from */
>> -if (avio_seek(pb, last_pos + 1, SEEK_SET) < 0)
>> -goto eof;
>> +if ((ret = avio_seek(pb, last_pos + 1, SEEK_SET)) < 0) {
>> +matroska->done = 1;
>> +return ret;
>>
>
> doesn't this generate a warning, returning an int64 from a function
> supposed to return an int?


avio_seek returns an int64, so at some point an int64 is going to be
converted to an int. I don't believe it will make a difference where,
especially since the values that will actually be returned (error codes)
should fit in an int.


>
> +}
>> id = avio_rb32(pb);
>>   @@ -760,7 +763,6 @@ static int matroska_resync(MatroskaDemuxContext
>> *matroska, int64_t last_pos)
>>   id = (id << 8) | avio_r8(pb);
>>   }
>>   -eof:
>>   matroska->done = 1;
>>   return AVERROR_EOF;
>>   }
>> @@ -3317,13 +3319,14 @@ static int 
>> matroska_parse_cluster(MatroskaDemuxContext
>> *matroska)
>>   static int matroska_read_packet(AVFormatContext *s, AVPacket *pkt)
>>   {
>>   MatroskaDemuxContext *matroska = s->priv_data;
>> +int ret = 0;
>> while (matroska_deliver_packet(matroska, pkt)) {
>>   int64_t pos = avio_tell(matroska->ctx->pb);
>>   if (matroska->done)
>> -return AVERROR_EOF;
>> +return (ret < 0) ? ret : AVERROR_EOF;
>>   if (matroska_parse_cluster(matroska) < 0)
>> -matroska_resync(matroska, pos);
>> +ret = matroska_resync(matroska, pos);
>>   }
>> return 0;
>>
>
> You might want to return ret instead of 0 here.


Done.


>
>
> --
> Ben
>
>
> ___
> 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