Re: [FFmpeg-devel] [PATCH] lavf/mov: atom box parsing return eof cause play fail

2017-12-07 Thread Michael Niedermayer 
On Thu, Dec 07, 2017 at 05:47:46PM +0800, tiejun.peng wrote:
> fix eof lead to play fail.
> 
> Signed-off-by: tiejun.peng 
> ---
>  libavformat/mov.c | 47 ---
>  1 file changed, 36 insertions(+), 11 deletions(-)

please split the addition of warning messages from the change to
EOF behavior

did you check that every EOF return case is safe to continue as if
no error occured ?
That change has quite wide effects possibly unless i misunderstand.


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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] lavf/mov: atom box parsing return eof cause play fail

2017-12-07 Thread tiejun.peng 
fix eof lead to play fail.

Signed-off-by: tiejun.peng 
---
 libavformat/mov.c | 47 ---
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index c901859..6c3567f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1991,8 +1991,10 @@ static int mov_read_stco(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->chunk_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STCO atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2522,8 +2524,10 @@ int ff_mov_read_stsd_entries(MOVContext *c, AVIOContext 
*pb, int entries)
 sc->stsd_count++;
 }
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSD atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2624,8 +2628,10 @@ static int mov_read_stsc(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->stsc_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSC atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2676,8 +2682,10 @@ static int mov_read_stps(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->stps_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STPS atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2723,8 +2731,10 @@ static int mov_read_stss(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->keyframe_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSS atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2808,8 +2818,10 @@ static int mov_read_stsz(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 av_free(buf);
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSZ atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2870,8 +2882,10 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 sc->duration_for_fps  += duration;
 sc->nb_frames_for_fps += total_sample_count;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STTS atom\n");
 return AVERROR_EOF;
+}
 
 st->nb_frames= total_sample_count;
 if (duration)
@@ -2948,8 +2962,10 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->ctts_count = ctts_count;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted CTTS atom\n");
 return AVERROR_EOF;
+}
 
 av_log(c->fc, AV_LOG_TRACE, "dts shift %d\n", sc->dts_shift);
 
@@ -2995,7 +3011,12 @@ static int mov_read_sbgp(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->rap_group_count = i;
 
-return pb->eof_reached ? AVERROR_EOF : 0;
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted SBGP atom\n");
+return AVERROR_EOF;
+}
+
+return 0;
 }
 
 /**
@@ -4720,8 +4741,10 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 fix_frag_index_entries(>frag_index, next_frag_index,
frag->track_id, entries);
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted TRUN atom\n");
 return AVERROR_EOF;
+}
 
 frag->implicit_offset = offset;
 
@@ -6609,7 +6632,9 @@ static int mov_read_header(AVFormatContext *s)
 do {
 if (mov->moov_retry)
 avio_seek(pb, 0, SEEK_SET);
-if ((err = mov_read_default(mov, pb, atom)) < 0) {
+/* EOF don't mean the file to play fail*/
+err = mov_read_default(mov, pb, atom);
+if (err < 0 && err != AVERROR_EOF) {
 av_log(s, AV_LOG_ERROR, "error reading header\n");
 mov_read_close(s);
 return err;
-- 
2.7.4



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

Re: [FFmpeg-devel] [PATCH] lavf/mov: atom box parsing return eof cause play fail

2017-12-06 Thread Carl Eugen Hoyos 
2017-12-06 9:25 GMT+01:00 tiejun.peng :

>  /* check MOV header */
>  do {
> -if (mov->moov_retry)
> -avio_seek(pb, 0, SEEK_SET);
> -if ((err = mov_read_default(mov, pb, atom)) < 0) {
> -av_log(s, AV_LOG_ERROR, "error reading header\n");
> -mov_read_close(s);
> -return err;
> -}
> +if (mov->moov_retry)
> +avio_seek(pb, 0, SEEK_SET);
> +/* EOF don't mean the file to play fail*/
> +err = mov_read_default(mov, pb, atom);
> +if (err < 0 && err != AVERROR_EOF) {
> +av_log(s, AV_LOG_ERROR, "error reading header\n");
> +mov_read_close(s);
> +return err;
> +}

Most of this change should be a separate (purely cosmetic) patch
to make reviewing the change (now and later) easier.

Feel free to only send your functional change (without the
re-indentation) now, the indentation can still be fixed.

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

[FFmpeg-devel] [PATCH] lavf/mov: atom box parsing return eof cause play fail

2017-12-06 Thread tiejun.peng 
fix eof lead to play fail.

Signed-off-by: tiejun.peng 
---
 libavformat/mov.c | 59 +++
 1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index c901859..870fdd6 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1991,8 +1991,10 @@ static int mov_read_stco(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->chunk_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STCO atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2522,8 +2524,10 @@ int ff_mov_read_stsd_entries(MOVContext *c, AVIOContext 
*pb, int entries)
 sc->stsd_count++;
 }
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSD atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2624,8 +2628,10 @@ static int mov_read_stsc(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->stsc_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSC atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2676,8 +2682,10 @@ static int mov_read_stps(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->stps_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STPS atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2723,8 +2731,10 @@ static int mov_read_stss(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->keyframe_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSS atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2808,8 +2818,10 @@ static int mov_read_stsz(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 av_free(buf);
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSZ atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2870,8 +2882,10 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 sc->duration_for_fps  += duration;
 sc->nb_frames_for_fps += total_sample_count;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STTS atom\n");
 return AVERROR_EOF;
+}
 
 st->nb_frames= total_sample_count;
 if (duration)
@@ -2948,8 +2962,10 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->ctts_count = ctts_count;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted CTTS atom\n");
 return AVERROR_EOF;
+}
 
 av_log(c->fc, AV_LOG_TRACE, "dts shift %d\n", sc->dts_shift);
 
@@ -2995,7 +3011,12 @@ static int mov_read_sbgp(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->rap_group_count = i;
 
-return pb->eof_reached ? AVERROR_EOF : 0;
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted SBGP atom\n");
+return AVERROR_EOF;
+}
+
+return 0;
 }
 
 /**
@@ -4720,8 +4741,10 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 fix_frag_index_entries(>frag_index, next_frag_index,
frag->track_id, entries);
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted TRUN atom\n");
 return AVERROR_EOF;
+}
 
 frag->implicit_offset = offset;
 
@@ -6607,13 +6630,15 @@ static int mov_read_header(AVFormatContext *s)
 
 /* check MOV header */
 do {
-if (mov->moov_retry)
-avio_seek(pb, 0, SEEK_SET);
-if ((err = mov_read_default(mov, pb, atom)) < 0) {
-av_log(s, AV_LOG_ERROR, "error reading header\n");
-mov_read_close(s);
-return err;
-}
+if (mov->moov_retry)
+avio_seek(pb, 0, SEEK_SET);
+/* EOF don't mean the file to play fail*/
+err = mov_read_default(mov, pb, atom);
+if (err < 0 && err != AVERROR_EOF) {
+av_log(s, AV_LOG_ERROR, "error reading header\n");
+mov_read_close(s);
+return err;
+}
 } while ((pb->seekable & AVIO_SEEKABLE_NORMAL) && !mov->found_moov && 
!mov->moov_retry++);
 if (!mov->found_moov) {
 av_log(s, AV_LOG_ERROR, "moov atom not found\n");
-- 
2.7.4



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

Re: [FFmpeg-devel] [PATCH] lavf/mov: atom box parsing return eof cause play fail

2017-12-05 Thread Michael Niedermayer 
On Tue, Dec 05, 2017 at 11:34:12AM +0800, tiejun.peng wrote:
> 1.add warning info about corrupted atom box parsing.
> 2.atom box parsing return eof cause mov_read_close called in mov_read_header
> and user have no chance to play the file.

i think someone should cleanup the english, it sounds a bit odd


[...]
> @@ -6609,7 +6632,8 @@ static int mov_read_header(AVFormatContext *s)
>  do {
>  if (mov->moov_retry)
>  avio_seek(pb, 0, SEEK_SET);
> -if ((err = mov_read_default(mov, pb, atom)) < 0) {
> +/* EOF don't mean the file to play fail*/

> +if ((err = mov_read_default(mov, pb, atom) && err != AVERROR_EOF) < 0) {

this does look wrong

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] lavf/mov: atom box parsing return eof cause play fail

2017-12-04 Thread tiejun.peng 
1.add warning info about corrupted atom box parsing.
2.atom box parsing return eof cause mov_read_close called in mov_read_header
and user have no chance to play the file.

Signed-off-by: tiejun.peng 
---
 libavformat/mov.c | 46 +++---
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index c901859..ef8b357 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1991,8 +1991,10 @@ static int mov_read_stco(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->chunk_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STCO atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2522,8 +2524,10 @@ int ff_mov_read_stsd_entries(MOVContext *c, AVIOContext 
*pb, int entries)
 sc->stsd_count++;
 }
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSD atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2624,8 +2628,10 @@ static int mov_read_stsc(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->stsc_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSC atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2676,8 +2682,10 @@ static int mov_read_stps(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->stps_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STPS atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2723,8 +2731,10 @@ static int mov_read_stss(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->keyframe_count = i;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSS atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2808,8 +2818,10 @@ static int mov_read_stsz(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 av_free(buf);
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STSZ atom\n");
 return AVERROR_EOF;
+}
 
 return 0;
 }
@@ -2870,8 +2882,10 @@ static int mov_read_stts(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 sc->duration_for_fps  += duration;
 sc->nb_frames_for_fps += total_sample_count;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted STTS atom\n");
 return AVERROR_EOF;
+}
 
 st->nb_frames= total_sample_count;
 if (duration)
@@ -2948,8 +2962,10 @@ static int mov_read_ctts(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->ctts_count = ctts_count;
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted CTTS atom\n");
 return AVERROR_EOF;
+}
 
 av_log(c->fc, AV_LOG_TRACE, "dts shift %d\n", sc->dts_shift);
 
@@ -2995,7 +3011,12 @@ static int mov_read_sbgp(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 sc->rap_group_count = i;
 
-return pb->eof_reached ? AVERROR_EOF : 0;
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted SBGP atom\n");
+return AVERROR_EOF;
+}
+
+return 0;
 }
 
 /**
@@ -4720,8 +4741,10 @@ static int mov_read_trun(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 fix_frag_index_entries(>frag_index, next_frag_index,
frag->track_id, entries);
 
-if (pb->eof_reached)
+if (pb->eof_reached) {
+av_log(c->fc, AV_LOG_WARNING, "reached eof, corrupted TRUN atom\n");
 return AVERROR_EOF;
+}
 
 frag->implicit_offset = offset;
 
@@ -6609,7 +6632,8 @@ static int mov_read_header(AVFormatContext *s)
 do {
 if (mov->moov_retry)
 avio_seek(pb, 0, SEEK_SET);
-if ((err = mov_read_default(mov, pb, atom)) < 0) {
+/* EOF don't mean the file to play fail*/
+if ((err = mov_read_default(mov, pb, atom) && err != AVERROR_EOF) < 0) {
 av_log(s, AV_LOG_ERROR, "error reading header\n");
 mov_read_close(s);
 return err;
-- 
2.7.4

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