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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160809/fa994b31/attachment.sig>