diff mbox series

[FFmpeg-devel,1/2] avformat/matroskaenc: Improve mimetype search

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

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt April 16, 2020, 2:18 a.m. UTC
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(-)

Comments

Andreas Rheinhardt April 19, 2020, 8:42 p.m. UTC | #1
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 mbox series

Patch

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 "