diff mbox

[FFmpeg-devel] avformat/movenc: fix recognization of cover image streams

Message ID 20180604143619.23263-1-timo.teras@iki.fi
State Accepted
Commit 2223811b015926fec68473a08016d40cea0989b2
Headers show

Commit Message

Timo Teräs June 4, 2018, 2:36 p.m. UTC 
For chapter images, the mov demux produces streams with disposition set
to attached_pic+timed_thumbnails. This patch fixes to properly recognize
streams that should be encoded as cover image (ones with only and only
attached_pic disposition set).

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
> ffmpeg should act close to what a inteligent user expects.
> For example a simple ffmpeg -i inputfile outputfile
> should produce a outputfile that results in similar presentation as the input
> when played.
>...
> It may be good to minimize the amount of exceptions for how streams are
> handled.

Agree. My question was more about the details how the disposition flags
should be handled, because there seems to be no accurate documentation.
Apparently the logic is to handle based on what demuxers produce.

This patch addresses how the cover image is detected, and should restore
earlier behaviour on copying files where demuxer supports more features
than current muxer.

This applies on top of the "[v2] avformat/movenc: properly handle cover
image codecs" patch. I can rebase the order if wanted, but it'll require
little work.

Hopefully just the above mentioned patch, and this one could be applied
in the order.

 libavformat/movenc.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

Comments

Timo Teräs June 11, 2018, 5:42 p.m. UTC | #1 
On Mon,  4 Jun 2018 17:36:19 +0300
Timo Teräs <timo.teras@iki.fi> wrote:

> For chapter images, the mov demux produces streams with disposition
> set to attached_pic+timed_thumbnails. This patch fixes to properly
> recognize streams that should be encoded as cover image (ones with
> only and only attached_pic disposition set).
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> ---
> > ffmpeg should act close to what a inteligent user expects.
> > For example a simple ffmpeg -i inputfile outputfile
> > should produce a outputfile that results in similar presentation as
> > the input when played.
> >...
> > It may be good to minimize the amount of exceptions for how streams
> > are handled.
> 
> Agree. My question was more about the details how the disposition
> flags should be handled, because there seems to be no accurate
> documentation. Apparently the logic is to handle based on what
> demuxers produce.
> 
> This patch addresses how the cover image is detected, and should
> restore earlier behaviour on copying files where demuxer supports
> more features than current muxer.
> 
> This applies on top of the "[v2] avformat/movenc: properly handle
> cover image codecs" patch. I can rebase the order if wanted, but
> it'll require little work.
> 
> Hopefully just the above mentioned patch, and this one could be
> applied in the order.

Ping. Any feedback for this or the other patch?
Michael Niedermayer June 12, 2018, 9:09 p.m. UTC | #2 
On Mon, Jun 04, 2018 at 05:36:19PM +0300, Timo Teräs wrote:
> For chapter images, the mov demux produces streams with disposition set
> to attached_pic+timed_thumbnails. This patch fixes to properly recognize
> streams that should be encoded as cover image (ones with only and only
> attached_pic disposition set).
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> ---
> > ffmpeg should act close to what a inteligent user expects.
> > For example a simple ffmpeg -i inputfile outputfile
> > should produce a outputfile that results in similar presentation as the input
> > when played.
> >...
> > It may be good to minimize the amount of exceptions for how streams are
> > handled.
> 
> Agree. My question was more about the details how the disposition flags
> should be handled, because there seems to be no accurate documentation.
> Apparently the logic is to handle based on what demuxers produce.
> 
> This patch addresses how the cover image is detected, and should restore
> earlier behaviour on copying files where demuxer supports more features
> than current muxer.
> 
> This applies on top of the "[v2] avformat/movenc: properly handle cover
> image codecs" patch. I can rebase the order if wanted, but it'll require
> little work.
> 
> Hopefully just the above mentioned patch, and this one could be applied
> in the order.

will apply the 2 patches

adding a fate test may make sense, the patches didnt change any so
its not tested

thx

[...]
Timo Teräs June 13, 2018, 2:57 p.m. UTC | #3 
On Tue, 12 Jun 2018 23:09:47 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Mon, Jun 04, 2018 at 05:36:19PM +0300, Timo Teräs wrote:
> > For chapter images, the mov demux produces streams with disposition
> > set to attached_pic+timed_thumbnails. This patch fixes to properly
> > recognize streams that should be encoded as cover image (ones with
> > only and only attached_pic disposition set).
> > 
> > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> > ---  
> > > ffmpeg should act close to what a inteligent user expects.
> > > For example a simple ffmpeg -i inputfile outputfile
> > > should produce a outputfile that results in similar presentation
> > > as the input when played.
> > >...
> > > It may be good to minimize the amount of exceptions for how
> > > streams are handled.  
> > 
> > Agree. My question was more about the details how the disposition
> > flags should be handled, because there seems to be no accurate
> > documentation. Apparently the logic is to handle based on what
> > demuxers produce.
> > 
> > This patch addresses how the cover image is detected, and should
> > restore earlier behaviour on copying files where demuxer supports
> > more features than current muxer.
> > 
> > This applies on top of the "[v2] avformat/movenc: properly handle
> > cover image codecs" patch. I can rebase the order if wanted, but
> > it'll require little work.
> > 
> > Hopefully just the above mentioned patch, and this one could be
> > applied in the order.  
> 
> will apply the 2 patches
> 
> adding a fate test may make sense, the patches didnt change any so
> its not tested

Thanks! Please cherry pick to 4.0-stable too.

I can look into adding fate test, but it might take a while as I am not
familiar with doing them yet.

Timo
Carl Eugen Hoyos June 13, 2018, 3:18 p.m. UTC | #4 
2018-06-13 16:57 GMT+02:00, Timo Teras <timo.teras@iki.fi>:
> On Tue, 12 Jun 2018 23:09:47 +0200
> Michael Niedermayer <michael@niedermayer.cc> wrote:
>
>> On Mon, Jun 04, 2018 at 05:36:19PM +0300, Timo Teräs wrote:
>> > For chapter images, the mov demux produces streams with disposition
>> > set to attached_pic+timed_thumbnails. This patch fixes to properly
>> > recognize streams that should be encoded as cover image (ones with
>> > only and only attached_pic disposition set).
>> >
>> > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
>> > ---
>> > > ffmpeg should act close to what a inteligent user expects.
>> > > For example a simple ffmpeg -i inputfile outputfile
>> > > should produce a outputfile that results in similar presentation
>> > > as the input when played.
>> > >...
>> > > It may be good to minimize the amount of exceptions for how
>> > > streams are handled.
>> >
>> > Agree. My question was more about the details how the disposition
>> > flags should be handled, because there seems to be no accurate
>> > documentation. Apparently the logic is to handle based on what
>> > demuxers produce.
>> >
>> > This patch addresses how the cover image is detected, and should
>> > restore earlier behaviour on copying files where demuxer supports
>> > more features than current muxer.
>> >
>> > This applies on top of the "[v2] avformat/movenc: properly handle
>> > cover image codecs" patch. I can rebase the order if wanted, but
>> > it'll require little work.
>> >
>> > Hopefully just the above mentioned patch, and this one could be
>> > applied in the order.
>>
>> will apply the 2 patches
>>
>> adding a fate test may make sense, the patches didnt change any so
>> its not tested
>
> Thanks! Please cherry pick to 4.0-stable too.

Doesn't this change behaviour?

Carl Eugen
Timo Teräs June 13, 2018, 4:11 p.m. UTC | #5 
On Wed, 13 Jun 2018 17:18:51 +0200
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2018-06-13 16:57 GMT+02:00, Timo Teras <timo.teras@iki.fi>:
> > On Tue, 12 Jun 2018 23:09:47 +0200
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >  
> >> On Mon, Jun 04, 2018 at 05:36:19PM +0300, Timo Teräs wrote:  
> >> > For chapter images, the mov demux produces streams with
> >> > disposition set to attached_pic+timed_thumbnails. This patch
> >> > fixes to properly recognize streams that should be encoded as
> >> > cover image (ones with only and only attached_pic disposition
> >> > set).
> >> >
> >> > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> >> > ---  
> >> > > ffmpeg should act close to what a inteligent user expects.
> >> > > For example a simple ffmpeg -i inputfile outputfile
> >> > > should produce a outputfile that results in similar
> >> > > presentation as the input when played.
> >> > >...
> >> > > It may be good to minimize the amount of exceptions for how
> >> > > streams are handled.  
> >> >
> >> > Agree. My question was more about the details how the disposition
> >> > flags should be handled, because there seems to be no accurate
> >> > documentation. Apparently the logic is to handle based on what
> >> > demuxers produce.
> >> >
> >> > This patch addresses how the cover image is detected, and should
> >> > restore earlier behaviour on copying files where demuxer supports
> >> > more features than current muxer.
> >> >
> >> > This applies on top of the "[v2] avformat/movenc: properly handle
> >> > cover image codecs" patch. I can rebase the order if wanted, but
> >> > it'll require little work.
> >> >
> >> > Hopefully just the above mentioned patch, and this one could be
> >> > applied in the order.  
> >>
> >> will apply the 2 patches
> >>
> >> adding a fate test may make sense, the patches didnt change any so
> >> its not tested  
> >
> > Thanks! Please cherry pick to 4.0-stable too.  
> 
> Doesn't this change behaviour?

Not really. They are bug fixes for the cover image thing introduced in
4.0. They in fact restore pre-4.0 behaviour for certain unusual stream
disposition types.

Technically, the first of these patches exposes another error in the
original implementation, and this second follow up patch fixes that.

I did ask if these two need to be reordered, but since the issue
affected only specific disposition types which are actually never
handled right in current code, it does not really make a big difference.

So over all, these two patches together, do not produce behaviour
difference and only fix/improve things.

Timo
diff mbox

Patch

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 4ba90135df..db266b7765 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -143,10 +143,17 @@  static int co64_required(const MOVTrack *track)
     return 0;
 }
 
+static int is_cover_image(const AVStream *st)
+{
+    /* Eg. AV_DISPOSITION_ATTACHED_PIC | AV_DISPOSITION_TIMED_THUMBNAILS
+     * is encoded as sparse video track */
+    return st && st->disposition == AV_DISPOSITION_ATTACHED_PIC;
+}
+
 static int rtp_hinting_needed(const AVStream *st)
 {
     /* Add hint tracks for each real audio and video stream */
-    if (st->disposition & AV_DISPOSITION_ATTACHED_PIC)
+    if (is_cover_image(st))
         return 0;
     return st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO ||
            st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO;
@@ -1568,7 +1575,7 @@  static int mov_find_codec_tag(AVFormatContext *s, MOVTrack *track)
 {
     int tag;
 
-    if (track->st->disposition & AV_DISPOSITION_ATTACHED_PIC)
+    if (is_cover_image(track->st))
         return ff_codec_get_tag(codec_cover_image_tags, track->par->codec_id);
 
     if (track->mode == MODE_MP4 || track->mode == MODE_PSP)
@@ -3443,10 +3450,8 @@  static int mov_write_covr(AVIOContext *pb, AVFormatContext *s)
 
     for (i = 0; i < s->nb_streams; i++) {
         MOVTrack *trk = &mov->tracks[i];
-        AVStream *st = s->streams[i];
 
-        if (!(st->disposition & AV_DISPOSITION_ATTACHED_PIC) ||
-            trk->cover_image.size <= 0)
+        if (!is_cover_image(trk->st) || trk->cover_image.size <= 0)
             continue;
 
         if (!pos) {
@@ -3989,15 +3994,13 @@  static int mov_write_isml_manifest(AVIOContext *pb, MOVMuxContext *mov, AVFormat
         AVStream *st = track->st;
         AVDictionaryEntry *lang = av_dict_get(st->metadata, "language", NULL,0);
 
-        if (track->par->codec_type == AVMEDIA_TYPE_VIDEO) {
+        if (track->par->codec_type == AVMEDIA_TYPE_VIDEO && !is_cover_image(st)) {
             type = "video";
         } else if (track->par->codec_type == AVMEDIA_TYPE_AUDIO) {
             type = "audio";
         } else {
             continue;
         }
-        if (st->disposition & AV_DISPOSITION_ATTACHED_PIC)
-            continue;
 
         props = (AVCPBProperties*)av_stream_get_side_data(track->st, AV_PKT_DATA_CPB_PROPERTIES, NULL);
 
@@ -4657,7 +4660,7 @@  static int mov_write_ftyp_tag(AVIOContext *pb, AVFormatContext *s)
 
     for (i = 0; i < s->nb_streams; i++) {
         AVStream *st = s->streams[i];
-        if (st->disposition & AV_DISPOSITION_ATTACHED_PIC)
+        if (is_cover_image(st))
             continue;
         if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO)
             has_video = 1;
@@ -4807,7 +4810,7 @@  static int mov_write_identification(AVIOContext *pb, AVFormatContext *s)
         int video_streams_nb = 0, audio_streams_nb = 0, other_streams_nb = 0;
         for (i = 0; i < s->nb_streams; i++) {
             AVStream *st = s->streams[i];
-            if (st->disposition & AV_DISPOSITION_ATTACHED_PIC)
+            if (is_cover_image(st))
                 continue;
             if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO)
                 video_streams_nb++;
@@ -4998,8 +5001,7 @@  static int mov_flush_fragment(AVFormatContext *s, int force)
         int buf_size, moov_size;
 
         for (i = 0; i < mov->nb_streams; i++)
-            if (!mov->tracks[i].entry &&
-                (i >= s->nb_streams || !(s->streams[i]->disposition & AV_DISPOSITION_ATTACHED_PIC)))
+            if (!mov->tracks[i].entry && !is_cover_image(mov->tracks[i].st))
                 break;
         /* Don't write the initial moov unless all tracks have data */
         if (i < mov->nb_streams && !force)
@@ -5581,21 +5583,19 @@  static int mov_write_packet(AVFormatContext *s, AVPacket *pkt)
 {
     MOVMuxContext *mov = s->priv_data;
     MOVTrack *trk;
-    AVStream *st;
 
     if (!pkt) {
         mov_flush_fragment(s, 1);
         return 1;
     }
 
-    st = s->streams[pkt->stream_index];
     trk = &mov->tracks[pkt->stream_index];
 
-    if (st->disposition & AV_DISPOSITION_ATTACHED_PIC) {
+    if (is_cover_image(trk->st)) {
         int ret;
 
-        if (st->nb_frames >= 1) {
-            if (st->nb_frames == 1)
+        if (trk->st->nb_frames >= 1) {
+            if (trk->st->nb_frames == 1)
                 av_log(s, AV_LOG_WARNING, "Got more than one picture in stream %d,"
                        " ignoring.\n", pkt->stream_index);
             return 0;
@@ -5854,7 +5854,7 @@  static void enable_tracks(AVFormatContext *s)
 
         if (st->codecpar->codec_type <= AVMEDIA_TYPE_UNKNOWN ||
             st->codecpar->codec_type >= AVMEDIA_TYPE_NB ||
-            st->disposition & AV_DISPOSITION_ATTACHED_PIC)
+            is_cover_image(st))
             continue;
 
         if (first[st->codecpar->codec_type] < 0)