Message ID | 20200416021804.17900-1-andreas.rheinhardt@gmail.com |
---|---|
State | Accepted |
Commit | 3589b3f2e217e78d16a92b372d95ce4a3f7df896 |
Headers | show |
Series | [FFmpeg-devel,1/2] avformat/matroskaenc: Improve mimetype search | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
Andreas Rheinhardt: > Use the mime_types of the corresponding AVCodecDescriptor instead of > tables specific to Matroska. The former are generally more encompassing: > They contain every item of the current lists except "text/plain" for > AV_CODEC_ID_TEXT and "binary" for AV_CODEC_ID_BIN_DATA. > > The former has been preserved by special-casing it while the latter is > a hack added in c9212abf so that the demuxer (which uses the same tables) > sets the appropriate CodecID for broken files ("binary" is not a correct > mime type at all); using it for the muxer was a mistake. The correct > mime type for AV_CODEC_ID_BIN_DATA is "application/octet-stream" and > this is what one gets from the AVCodecDescriptor. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > I also have a patch like this for the Matroska demuxer; it would make > the mime_tables in matroska.c completely superfluous. But there are two > things that need to be fixed before this can happen: > > 1. The Matroska muxer must be able to write video streams with > attached pic disposition as attachment. (This is currently broken and if > more attachments will be exported as attached picture video streams (as > will happen because the AVCodecDescriptor list contains entries (e.g. > WebP) that the current list does not contain), then more simple > Matroska->Matroska remuxing scenarios will be affected by this.) > > 2. The Matroska demuxer does not set the dimensions of attached pictures > (because they are not available at the container level). So if no decoder > is available, these values will never be set. But init_muxer() in mux.c > checks for valid dimensions (unless the format is of AVFMT_NODIMENSIONS > type which Matroska is not (strangely not even the null or the hash > muxers have this set)) and this breaks e.g. remuxing Matroska->Matroska. > Given that e.g. the flac muxer needs the dimensions for attached > pictures, one cannot simply omit the check for attached picture streams. > The only solution I can think of is adding another flag which says that > the format does not need dimensions for attached pictures. > > Of course, 1. needs to be fixed before the proposed solution for 2. > makes any sense (at least for Matroska, I don't know if there are other > formats that are affected by this). > > libavformat/matroskaenc.c | 16 +++++----------- > 1 file changed, 5 insertions(+), 11 deletions(-) > > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c > index d3256d8f5d..e1ddf366d4 100644 > --- a/libavformat/matroskaenc.c > +++ b/libavformat/matroskaenc.c > @@ -1667,17 +1667,11 @@ static int mkv_write_attachments(AVFormatContext *s) > if (t = av_dict_get(st->metadata, "mimetype", NULL, 0)) > mimetype = t->value; > else if (st->codecpar->codec_id != AV_CODEC_ID_NONE ) { > - int i; > - for (i = 0; ff_mkv_mime_tags[i].id != AV_CODEC_ID_NONE; i++) > - if (ff_mkv_mime_tags[i].id == st->codecpar->codec_id) { > - mimetype = ff_mkv_mime_tags[i].str; > - break; > - } > - for (i = 0; ff_mkv_image_mime_tags[i].id != AV_CODEC_ID_NONE; i++) > - if (ff_mkv_image_mime_tags[i].id == st->codecpar->codec_id) { > - mimetype = ff_mkv_image_mime_tags[i].str; > - break; > - } > + const AVCodecDescriptor *desc = avcodec_descriptor_get(st->codecpar->codec_id); > + if (desc && desc->mime_types) { > + mimetype = desc->mime_types[0]; > + } else if (st->codecpar->codec_id = AV_CODEC_ID_TEXT) > + mimetype = "text/plain"; > } > if (!mimetype) { > av_log(s, AV_LOG_ERROR, "Attachment stream %d has no mimetype tag and " > Will apply tomorrow if there are no objections. - Andreas
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index d3256d8f5d..e1ddf366d4 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -1667,17 +1667,11 @@ static int mkv_write_attachments(AVFormatContext *s) if (t = av_dict_get(st->metadata, "mimetype", NULL, 0)) mimetype = t->value; else if (st->codecpar->codec_id != AV_CODEC_ID_NONE ) { - int i; - for (i = 0; ff_mkv_mime_tags[i].id != AV_CODEC_ID_NONE; i++) - if (ff_mkv_mime_tags[i].id == st->codecpar->codec_id) { - mimetype = ff_mkv_mime_tags[i].str; - break; - } - for (i = 0; ff_mkv_image_mime_tags[i].id != AV_CODEC_ID_NONE; i++) - if (ff_mkv_image_mime_tags[i].id == st->codecpar->codec_id) { - mimetype = ff_mkv_image_mime_tags[i].str; - break; - } + const AVCodecDescriptor *desc = avcodec_descriptor_get(st->codecpar->codec_id); + if (desc && desc->mime_types) { + mimetype = desc->mime_types[0]; + } else if (st->codecpar->codec_id = AV_CODEC_ID_TEXT) + mimetype = "text/plain"; } if (!mimetype) { av_log(s, AV_LOG_ERROR, "Attachment stream %d has no mimetype tag and "
Use the mime_types of the corresponding AVCodecDescriptor instead of tables specific to Matroska. The former are generally more encompassing: They contain every item of the current lists except "text/plain" for AV_CODEC_ID_TEXT and "binary" for AV_CODEC_ID_BIN_DATA. The former has been preserved by special-casing it while the latter is a hack added in c9212abf so that the demuxer (which uses the same tables) sets the appropriate CodecID for broken files ("binary" is not a correct mime type at all); using it for the muxer was a mistake. The correct mime type for AV_CODEC_ID_BIN_DATA is "application/octet-stream" and this is what one gets from the AVCodecDescriptor. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- I also have a patch like this for the Matroska demuxer; it would make the mime_tables in matroska.c completely superfluous. But there are two things that need to be fixed before this can happen: 1. The Matroska muxer must be able to write video streams with attached pic disposition as attachment. (This is currently broken and if more attachments will be exported as attached picture video streams (as will happen because the AVCodecDescriptor list contains entries (e.g. WebP) that the current list does not contain), then more simple Matroska->Matroska remuxing scenarios will be affected by this.) 2. The Matroska demuxer does not set the dimensions of attached pictures (because they are not available at the container level). So if no decoder is available, these values will never be set. But init_muxer() in mux.c checks for valid dimensions (unless the format is of AVFMT_NODIMENSIONS type which Matroska is not (strangely not even the null or the hash muxers have this set)) and this breaks e.g. remuxing Matroska->Matroska. Given that e.g. the flac muxer needs the dimensions for attached pictures, one cannot simply omit the check for attached picture streams. The only solution I can think of is adding another flag which says that the format does not need dimensions for attached pictures. Of course, 1. needs to be fixed before the proposed solution for 2. makes any sense (at least for Matroska, I don't know if there are other formats that are affected by this). libavformat/matroskaenc.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)