diff mbox

[FFmpeg-devel] lavf/mov: ignore edit list with duration equals to 0 for covers art

Message ID 20170118103841.13283-1-matthieu.bouron@gmail.com
State New
Headers show

Commit Message

Matthieu Bouron Jan. 18, 2017, 10:38 a.m. UTC 
Discards edit list with duration equals to 0 for video streams with only
one frame and avoid discarding covers art muxed as a single frame video
stream.
---
Hello,

The following patch discards single edit list with duration equals to 0 for video streams with only one frame which prevents discarding covers art muxed as a single frame video stream.

The patch can be extended to make it always discards single edit list with duration equals to 0 (even if the video streams has more than one sample).
What do you think ?

Matthieu
---
 libavformat/mov.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Yusuke Nakamura Jan. 18, 2017, 7:55 p.m. UTC | #1 
2017-01-18 19:38 GMT+09:00 Matthieu Bouron <matthieu.bouron@gmail.com>:

> Discards edit list with duration equals to 0 for video streams with only
> one frame and avoid discarding covers art muxed as a single frame video
> stream.
> ---
> Hello,
>
> The following patch discards single edit list with duration equals to 0
> for video streams with only one frame which prevents discarding covers art
> muxed as a single frame video stream.
>
> The patch can be extended to make it always discards single edit list with
> duration equals to 0 (even if the video streams has more than one sample).
> What do you think ?
>
> Matthieu
> ---
>  libavformat/mov.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index d1b929174d..88ffd0e5f2 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -2988,6 +2988,17 @@ static void mov_fix_index(MOVContext *mov, AVStream
> *st)
>      if (!msc->elst_data || msc->elst_count <= 0 || nb_old <= 0) {
>          return;
>      }
> +
> +    // Discard edit list with duration equals to 0 for video streams with
> only
> +    // one frame and avoid discarding covers art muxed as a single frame
> video
> +    // stream
> +    if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
> +        msc->chunk_count == 1 &&
> +        msc->elst_count == 1 &&
> +        msc->elst_data[0].duration == 0) {
>

This can be false positive since segment_duration=0 could be used for
implicit segment_duration when movie fragments. The spec explicitly says
that only when initial movie has no samples but
w16161-14496-12-DefectReport-R4.docx implies that the 14496-12 spec will
also apply it to more generic cases. And why msc->chunk_count == 1?  The
chunk_count is the number of chunks but not samples.


> +        return;
> +    }
> +
>      // Clean AVStream from traces of old index
>      st->index_entries = NULL;
>      st->index_entries_allocated_size = 0;
> --
> 2.11.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Matthieu Bouron Jan. 19, 2017, 9:33 a.m. UTC | #2 
On Thu, Jan 19, 2017 at 04:55:22AM +0900, Yusuke Nakamura wrote:
> 2017-01-18 19:38 GMT+09:00 Matthieu Bouron <matthieu.bouron@gmail.com>:
> 
> > Discards edit list with duration equals to 0 for video streams with only
> > one frame and avoid discarding covers art muxed as a single frame video
> > stream.
> > ---
> > Hello,
> >
> > The following patch discards single edit list with duration equals to 0
> > for video streams with only one frame which prevents discarding covers art
> > muxed as a single frame video stream.
> >
> > The patch can be extended to make it always discards single edit list with
> > duration equals to 0 (even if the video streams has more than one sample).
> > What do you think ?
> >
> > Matthieu
> > ---
> >  libavformat/mov.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index d1b929174d..88ffd0e5f2 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -2988,6 +2988,17 @@ static void mov_fix_index(MOVContext *mov, AVStream
> > *st)
> >      if (!msc->elst_data || msc->elst_count <= 0 || nb_old <= 0) {
> >          return;
> >      }
> > +
> > +    // Discard edit list with duration equals to 0 for video streams with
> > only
> > +    // one frame and avoid discarding covers art muxed as a single frame
> > video
> > +    // stream
> > +    if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
> > +        msc->chunk_count == 1 &&
> > +        msc->elst_count == 1 &&
> > +        msc->elst_data[0].duration == 0) {
> >
> 
> This can be false positive since segment_duration=0 could be used for
> implicit segment_duration when movie fragments. The spec explicitly says
> that only when initial movie has no samples but
> w16161-14496-12-DefectReport-R4.docx implies that the 14496-12 spec will
> also apply it to more generic cases. And why msc->chunk_count == 1?  The
> chunk_count is the number of chunks but not samples.
> 

If I understand correctly segment_duration=0 means implicit duration for
all cases (fragmented / non-fragmented) ?  Does that mean we should
discard the duration but honor the rest of the elst fields ?

Regarding the use of msc->chunk_count I wrongly assumed that one chunk ==
one video frame. I used originally st->nb_index_entries but I'm not sure
if it's right though.

Thanks,
Matthieu

[...]
Sasi Inguva Jan. 19, 2017, 6:33 p.m. UTC | #3 
According to  spec ( ISO-IEC-15444-12 ) specifying edit list duration 0,
makes sense for MP4F files
"A non‐empty edit may insert a portion of the media timeline that is not
present in the initial movie, and
is present only in subsequent movie fragments. Particularly in an empty
initial movie of a fragmented
movie file (when there are no media samples yet present), the
segment_duration of this edit may be
zero, whereupon the edit provides the offset from media composition time to
movie presentation time,
for the movie and subsequent movie fragments. It is recommended that such
an edit be used to
establish a presentation time of 0 for the first presented sample, when
composition offsets are used"

I was contacted with such a file previously and I tried playing that  in
Quicktime in mac to see its behavior.  When I played the video with
Quicktime 10.4 it showed the poster frame.  When I played it with Quicktime
7 , it showed black screen. Even in Quicktime 10.4 when you export the
video to reencode it, the output file has only audio.

However your hack seems to be specialized enough, so as not to hurt other
edit list cases. You just need to check that the file is not Fragmented MP4
, and need to check for st->nb_index_entries == 1 instead of
chunk_count==1.

On Thu, Jan 19, 2017 at 1:33 AM, Matthieu Bouron <matthieu.bouron@gmail.com>
wrote:

> On Thu, Jan 19, 2017 at 04:55:22AM +0900, Yusuke Nakamura wrote:
> > 2017-01-18 19:38 GMT+09:00 Matthieu Bouron <matthieu.bouron@gmail.com>:
> >
> > > Discards edit list with duration equals to 0 for video streams with
> only
> > > one frame and avoid discarding covers art muxed as a single frame video
> > > stream.
> > > ---
> > > Hello,
> > >
> > > The following patch discards single edit list with duration equals to 0
> > > for video streams with only one frame which prevents discarding covers
> art
> > > muxed as a single frame video stream.
> > >
> > > The patch can be extended to make it always discards single edit list
> with
> > > duration equals to 0 (even if the video streams has more than one
> sample).
> > > What do you think ?
> > >
> > > Matthieu
> > > ---
> > >  libavformat/mov.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > index d1b929174d..88ffd0e5f2 100644
> > > --- a/libavformat/mov.c
> > > +++ b/libavformat/mov.c
> > > @@ -2988,6 +2988,17 @@ static void mov_fix_index(MOVContext *mov,
> AVStream
> > > *st)
> > >      if (!msc->elst_data || msc->elst_count <= 0 || nb_old <= 0) {
> > >          return;
> > >      }
> > > +
> > > +    // Discard edit list with duration equals to 0 for video streams
> with
> > > only
> > > +    // one frame and avoid discarding covers art muxed as a single
> frame
> > > video
> > > +    // stream
> > > +    if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
> > > +        msc->chunk_count == 1 &&
> > > +        msc->elst_count == 1 &&
> > > +        msc->elst_data[0].duration == 0) {
> > >
> >
> > This can be false positive since segment_duration=0 could be used for
> > implicit segment_duration when movie fragments. The spec explicitly says
> > that only when initial movie has no samples but
> > w16161-14496-12-DefectReport-R4.docx implies that the 14496-12 spec will
> > also apply it to more generic cases. And why msc->chunk_count == 1?  The
> > chunk_count is the number of chunks but not samples.
> >
>
> If I understand correctly segment_duration=0 means implicit duration for
> all cases (fragmented / non-fragmented) ?  Does that mean we should
> discard the duration but honor the rest of the elst fields ?
>
> Regarding the use of msc->chunk_count I wrongly assumed that one chunk ==
> one video frame. I used originally st->nb_index_entries but I'm not sure
> if it's right though.
>
> Thanks,
> Matthieu
>
> [...]
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Yusuke Nakamura Jan. 19, 2017, 9:17 p.m. UTC | #4 
2017-01-19 18:33 GMT+09:00 Matthieu Bouron <matthieu.bouron@gmail.com>:

> On Thu, Jan 19, 2017 at 04:55:22AM +0900, Yusuke Nakamura wrote:
> > 2017-01-18 19:38 GMT+09:00 Matthieu Bouron <matthieu.bouron@gmail.com>:
> >
> > > Discards edit list with duration equals to 0 for video streams with
> only
> > > one frame and avoid discarding covers art muxed as a single frame video
> > > stream.
> > > ---
> > > Hello,
> > >
> > > The following patch discards single edit list with duration equals to 0
> > > for video streams with only one frame which prevents discarding covers
> art
> > > muxed as a single frame video stream.
> > >
> > > The patch can be extended to make it always discards single edit list
> with
> > > duration equals to 0 (even if the video streams has more than one
> sample).
> > > What do you think ?
> > >
> > > Matthieu
> > > ---
> > >  libavformat/mov.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > > index d1b929174d..88ffd0e5f2 100644
> > > --- a/libavformat/mov.c
> > > +++ b/libavformat/mov.c
> > > @@ -2988,6 +2988,17 @@ static void mov_fix_index(MOVContext *mov,
> AVStream
> > > *st)
> > >      if (!msc->elst_data || msc->elst_count <= 0 || nb_old <= 0) {
> > >          return;
> > >      }
> > > +
> > > +    // Discard edit list with duration equals to 0 for video streams
> with
> > > only
> > > +    // one frame and avoid discarding covers art muxed as a single
> frame
> > > video
> > > +    // stream
> > > +    if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
> > > +        msc->chunk_count == 1 &&
> > > +        msc->elst_count == 1 &&
> > > +        msc->elst_data[0].duration == 0) {
> > >
> >
> > This can be false positive since segment_duration=0 could be used for
> > implicit segment_duration when movie fragments. The spec explicitly says
> > that only when initial movie has no samples but
> > w16161-14496-12-DefectReport-R4.docx implies that the 14496-12 spec will
> > also apply it to more generic cases. And why msc->chunk_count == 1?  The
> > chunk_count is the number of chunks but not samples.
> >
>
> If I understand correctly segment_duration=0 means implicit duration for
> all cases (fragmented / non-fragmented) ?  Does that mean we should
> discard the duration but honor the rest of the elst fields ?
>

Dunno if it also applies to non-fragmented movie but an example of the
current spec does not specify it's fragmented or not. Also I noticed the
latest File-format Meeting Report, which is found at Mp4-sys ML and a MPEG
meeting is held in this week, and it says

"Proposal #3, edit list: The question is whether a zero-length edit in a
non-empty movie means anything. It probably means an insert of zero
duration, which means it needs adjusting if de-fragmenting. This needs
saying in the spec.; update the defect report."

This time I can't insist which is correct or not. :<



> Regarding the use of msc->chunk_count I wrongly assumed that one chunk ==
> one video frame. I used originally st->nb_index_entries but I'm not sure
> if it's right though.
>
> Thanks,
> Matthieu
>
> [...]
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Matthieu Bouron Jan. 19, 2017, 10:18 p.m. UTC | #5 
On Thu, Jan 19, 2017 at 10:33:25AM -0800, Sasi Inguva wrote:
> According to  spec ( ISO-IEC-15444-12 ) specifying edit list duration 0,
> makes sense for MP4F files
> "A non‐empty edit may insert a portion of the media timeline that is not
> present in the initial movie, and
> is present only in subsequent movie fragments. Particularly in an empty
> initial movie of a fragmented
> movie file (when there are no media samples yet present), the
> segment_duration of this edit may be
> zero, whereupon the edit provides the offset from media composition time to
> movie presentation time,
> for the movie and subsequent movie fragments. It is recommended that such
> an edit be used to
> establish a presentation time of 0 for the first presented sample, when
> composition offsets are used"
> 
> I was contacted with such a file previously and I tried playing that  in
> Quicktime in mac to see its behavior.  When I played the video with
> Quicktime 10.4 it showed the poster frame.  When I played it with Quicktime
> 7 , it showed black screen. Even in Quicktime 10.4 when you export the
> video to reencode it, the output file has only audio.
> 
> However your hack seems to be specialized enough, so as not to hurt other
> edit list cases. You just need to check that the file is not Fragmented MP4
> , and need to check for st->nb_index_entries == 1 instead of
> chunk_count==1.

I'm looking for a way to check the fragmented case. Would introducing
MOVContext.found_moof (like for moov) and set it in mov_read_moof OK ?
The other variables from MOVContext.fragment do not look like good
candidates to perform the fragmented check against.

[...]
Sasi Inguva Jan. 20, 2017, 1:25 a.m. UTC | #6 
The mov_fix_index is executed inside mov_read_trak . Normally 'moof' atom
is after the 'trak' atom in mp4f file. So the method of looking whether
moof atom exists won't work.

On Thu, Jan 19, 2017 at 2:18 PM, Matthieu Bouron <matthieu.bouron@gmail.com>
wrote:

> On Thu, Jan 19, 2017 at 10:33:25AM -0800, Sasi Inguva wrote:
> > According to  spec ( ISO-IEC-15444-12 ) specifying edit list duration 0,
> > makes sense for MP4F files
> > "A non‐empty edit may insert a portion of the media timeline that is not
> > present in the initial movie, and
> > is present only in subsequent movie fragments. Particularly in an empty
> > initial movie of a fragmented
> > movie file (when there are no media samples yet present), the
> > segment_duration of this edit may be
> > zero, whereupon the edit provides the offset from media composition time
> to
> > movie presentation time,
> > for the movie and subsequent movie fragments. It is recommended that such
> > an edit be used to
> > establish a presentation time of 0 for the first presented sample, when
> > composition offsets are used"
> >
> > I was contacted with such a file previously and I tried playing that  in
> > Quicktime in mac to see its behavior.  When I played the video with
> > Quicktime 10.4 it showed the poster frame.  When I played it with
> Quicktime
> > 7 , it showed black screen. Even in Quicktime 10.4 when you export the
> > video to reencode it, the output file has only audio.
> >
> > However your hack seems to be specialized enough, so as not to hurt other
> > edit list cases. You just need to check that the file is not Fragmented
> MP4
> > , and need to check for st->nb_index_entries == 1 instead of
> > chunk_count==1.
>
> I'm looking for a way to check the fragmented case. Would introducing
> MOVContext.found_moof (like for moov) and set it in mov_read_moof OK ?
> The other variables from MOVContext.fragment do not look like good
> candidates to perform the fragmented check against.
>
> [...]
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
diff mbox

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index d1b929174d..88ffd0e5f2 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2988,6 +2988,17 @@  static void mov_fix_index(MOVContext *mov, AVStream *st)
     if (!msc->elst_data || msc->elst_count <= 0 || nb_old <= 0) {
         return;
     }
+
+    // Discard edit list with duration equals to 0 for video streams with only
+    // one frame and avoid discarding covers art muxed as a single frame video
+    // stream
+    if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO &&
+        msc->chunk_count == 1 &&
+        msc->elst_count == 1 &&
+        msc->elst_data[0].duration == 0) {
+        return;
+    }
+
     // Clean AVStream from traces of old index
     st->index_entries = NULL;
     st->index_entries_allocated_size = 0;