Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#4423 closed defect (fixed)

Set AV_DISPOSITION_ATTACHED_PIC for MKV Cover Art

Reported by: malaterre Owned by:
Priority: normal Component: avformat
Version: git-master Keywords: mkv
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

minidlna currently uses the following piece of code to find Cover Art:

https://sourceforge.net/p/minidlna/git/ci/master/tree/libav.h#l171
[...]

if (s->disposition & AV_DISPOSITION_ATTACHED_PIC &&

s->codec->codec_id == AV_CODEC_ID_MJPEG)

[...]

However the flag AV_DISPOSITION_ATTACHED_PIC is never set on MKV. Steps:

Go here for MKV details on Cover Art mecanism: http://matroska.org/technical/cover_art/index.html

Download sample:

$ wget https://sourceforge.net/projects/matroska/files/test_files/cover_art.mkv
$ ffprobe -show_streams cover_art.mkv
[...]
[STREAM]
index=5
codec_name=mjpeg
codec_long_name=MJPEG (Motion JPEG)
profile=unknown
codec_type=attachment
codec_time_base=0/1
codec_tag_string=[0][0][0][0]
codec_tag=0x0000
id=N/A
r_frame_rate=0/0
avg_frame_rate=0/0
time_base=1/90000
start_pts=0
start_time=0.000000
duration_ts=14040000
duration=156.000000
bit_rate=N/A
max_bit_rate=N/A
bits_per_raw_sample=N/A
nb_frames=N/A
nb_read_frames=N/A
nb_read_packets=N/A
DISPOSITION:default=0
DISPOSITION:dub=0
DISPOSITION:original=0
DISPOSITION:comment=0
DISPOSITION:lyrics=0
DISPOSITION:karaoke=0
DISPOSITION:forced=0
DISPOSITION:hearing_impaired=0
DISPOSITION:visual_impaired=0
DISPOSITION:clean_effects=0
DISPOSITION:attached_pic=0
TAG:filename=small_cover_land.jpg
TAG:mimetype=image/jpeg
STREAM

Change History (9)

comment:1 by Hendrik, 9 years ago

Matroska uses codec_type == AVMEDIA_TYPE_ATTACHMENT, with mimetype and filename in metadata, since it can carry any generic attachments, and not just images. The code should probably be updated to use that in addition to the current check.

in reply to:  description comment:2 by Carl Eugen Hoyos, 9 years ago

Replying to malaterre:

minidlna currently uses the following piece of code to find Cover Art:

https://sourceforge.net/p/minidlna/git/ci/master/tree/libav.h#l171
[...]

if (s->disposition & AV_DISPOSITION_ATTACHED_PIC &&

s->codec->codec_id == AV_CODEC_ID_MJPEG)

Shouldn't this be:

    if ((s->disposition & AV_DISPOSITION_ATTACHED_PIC || s->codec->codec_type == AVMEDIA_TYPE_ATTACHMENT) &&
        s->codec->codec_id == AV_CODEC_ID_MJPEG)

comment:3 by malaterre, 9 years ago

I believe the comment #4423#comment:1 is correct. Having proper support for AV_DISPOSITION_ATTACHED_PIC in MKV will allow transcoding of Cover Art to and from MP4.

Last edited 9 years ago by malaterre (previous) (diff)

comment:4 by Hendrik, 9 years ago

My comment means you should update minidlna code to accept codec_type == AVMEDIA_TYPE_ATTACHMENT. Matroska attachments are very generic, and the attached pic disposition requires quite specific handling, so trying to make Matroska support that would end up in quite a lot of special-case hackery.

comment:5 by gjdfgh, 9 years ago

Cover art in Matroska is never a video stream. In libavformat they are - so it requires new code to export them correctly, which to my knowledge, libavformat doesn't do yet.

What the hell does this have to do with the codec? Don't compare codec IDs. It has nothing to do with it in this case. Just stop.

comment:6 by gjdfgh, 9 years ago

PS: I might send a patch to export these correctly some time.

comment:7 by gjdfgh, 9 years ago

Patch sent: [PATCH] matroskadec: export cover art correctly

Seems like the codec IDs were just looked up from the mime type (instead of frankendetected by libavformat/utils.c), so that was sufficiently correct, except there are more codecs than just jpg.

comment:8 by gjdfgh, 9 years ago

Resolution: fixed
Status: newclosed

The patch was merged. mkv coverart should now behave exactly as with other file formats in libavformat.

comment:9 by Carl Eugen Hoyos, 9 years ago

Keywords: mkv added
Version: 2.6.1git-master
Note: See TracTickets for help on using tickets.