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

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

On Tue, Aug 02, 2016 at 01:11:57PM -0700, Sophia Wang wrote:
> Signed-off-by: Sophia Wang <skw@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;
> +        }
>      }

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

[...]

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,

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 <george@nsup.org> 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
>
>