[FFmpeg-devel] avformat/rawenc: check stream type

Submitted by Gyan on April 6, 2018, 3:04 p.m.

Details

Message ID 90b8080b-bccd-81cb-04d4-935f2b1c8a8f@gmail.com
State New
Headers show

Commit Message

Gyan April 6, 2018, 3:04 p.m.
On 4/5/2018 12:09 AM, Michael Niedermayer wrote:
> 
> This does not work
> breaks fate-unknown_layout-ac3
> 
> also the tests mayb too strict, there are similar codec_ids like mpeg1/2
> or jpeg variants which are all basically the same from a muxers point of view

Revised patch passes FATE. Set looser conditions for mpeg-1/2/4 and jpeg 
muxer.

Regards,
Gyan
From a4ae3eb20af9a4c6e5feb201347b5b044541c59b Mon Sep 17 00:00:00 2001
From: Gyan Doshi <gyandoshi@gmail.com>
Date: Fri, 6 Apr 2018 20:21:57 +0530
Subject: [PATCH v2] avformat/rawenc: check stream type

Validate codec of stream to be muxed except for data muxer.
---
 libavformat/rawenc.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

Gyan April 9, 2018, 4:56 p.m.
On 4/6/2018 8:34 PM, Gyan Doshi wrote:
> 
> On 4/5/2018 12:09 AM, Michael Niedermayer wrote:
>>
>> This does not work
>> breaks fate-unknown_layout-ac3
>>
>> also the tests mayb too strict, there are similar codec_ids like mpeg1/2
>> or jpeg variants which are all basically the same from a muxers point 
>> of view
> 
> Revised patch passes FATE. Set looser conditions for mpeg-1/2/4 and jpeg 
> muxer.

Ping.
Michael Niedermayer April 9, 2018, 11:19 p.m.
On Fri, Apr 06, 2018 at 08:34:12PM +0530, Gyan Doshi wrote:
> 
> On 4/5/2018 12:09 AM, Michael Niedermayer wrote:
> >
> >This does not work
> >breaks fate-unknown_layout-ac3
> >
> >also the tests mayb too strict, there are similar codec_ids like mpeg1/2
> >or jpeg variants which are all basically the same from a muxers point of view
> 
> Revised patch passes FATE. Set looser conditions for mpeg-1/2/4 and jpeg
> muxer.
> 
> Regards,
> Gyan

>  rawenc.c |   36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 5aaae3c1bf40a983cff15bc2fc603ac214d9437a  v2-0001-avformat-rawenc-check-stream-type.patch
> From a4ae3eb20af9a4c6e5feb201347b5b044541c59b Mon Sep 17 00:00:00 2001
> From: Gyan Doshi <gyandoshi@gmail.com>
> Date: Fri, 6 Apr 2018 20:21:57 +0530
> Subject: [PATCH v2] avformat/rawenc: check stream type
> 
> Validate codec of stream to be muxed except for data muxer.
> ---
>  libavformat/rawenc.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/libavformat/rawenc.c b/libavformat/rawenc.c
> index 809ca23b1a..19028329f7 100644
> --- a/libavformat/rawenc.c
> +++ b/libavformat/rawenc.c
> @@ -34,12 +34,48 @@ int ff_raw_write_packet(AVFormatContext *s, AVPacket *pkt)
>  
>  static int force_one_stream(AVFormatContext *s)
>  {
> +enum AVCodecID id;
> +const char *idname;

indention is wrong


> +
>      if (s->nb_streams != 1) {
>          av_log(s, AV_LOG_ERROR, "%s files have exactly one stream\n",
>                 s->oformat->name);
>          return AVERROR(EINVAL);
>      }
> +
> +    if (!strcmp("data", s->oformat->name))
> +        return 0;
> +
> +id = s->streams[0]->codecpar->codec_id;
> +idname = avcodec_get_name(id);

same


> +
> +    switch(s->streams[0]->codecpar->codec_type) {
> +        case AVMEDIA_TYPE_AUDIO:
> +            if (s->oformat->audio_codec != id)
> +                goto fail;
> +            break;
> +        case AVMEDIA_TYPE_VIDEO:
> +            if (strstr(s->oformat->name, "mpeg") != NULL) {
> +                if (strstr(idname, "mpeg") == NULL)
> +                    goto fail;
> +            } else if (strstr(s->oformat->name, "m4v") != NULL) {
> +                       if (strstr(idname, "mpeg4") == NULL)
> +                           goto fail;
> +            } else if (strstr(s->oformat->name, "jpeg") != NULL) {
> +                       if (strstr(idname, "jpeg") == NULL)
> +                           goto fail;
> +            } else if (s->oformat->video_codec != id)
> +                       goto fail;
> +            break;

iam not sure these cross similarity stuff should be hardcoded here.
It seems painfull to have to maintain this.
also the ones i mentioned might not be all cases

To implement generic "codec supported" checks, muxers would have to
list what they support.
There are 3 ways i know how this can be done in the current API
1. the audio_codec / video_codec / ... fields
2. the codec_tag array (this was in fact IIRC one of the intended uses of it)
3. the querry_format callback

thanks

[...]
Gyan April 10, 2018, 1:51 p.m.
On 4/10/2018 4:49 AM, Michael Niedermayer wrote:

> To implement generic "codec supported" checks, muxers would have to
> list what they support.
> There are 3 ways i know how this can be done in the current API
> 1. the audio_codec / video_codec / ... fields
> 2. the codec_tag array (this was in fact IIRC one of the intended uses of it)
> 3. the querry_format callback

These are raw single stream muxers - most only dump packets and _should_ 
accept exactly one codec_id. A couple may insert bitstream filters. Only 
one writes a trailer. So none of them have a codec_tag member or a 
query_codec callback. Personally, I agree that the current method is 
inelegant but these "muxers" are stable and these tests shouldn't need 
changes. But I can go the query_codec route if you advise it.

Regards,
Gyan
Michael Niedermayer April 10, 2018, 10:17 p.m.
On Tue, Apr 10, 2018 at 07:21:29PM +0530, Gyan Doshi wrote:
> 
> 
> On 4/10/2018 4:49 AM, Michael Niedermayer wrote:
> 
> >To implement generic "codec supported" checks, muxers would have to
> >list what they support.
> >There are 3 ways i know how this can be done in the current API
> >1. the audio_codec / video_codec / ... fields
> >2. the codec_tag array (this was in fact IIRC one of the intended uses of it)
> >3. the querry_format callback
> 
> These are raw single stream muxers - most only dump packets and _should_
> accept exactly one codec_id. A couple may insert bitstream filters. Only one
> writes a trailer. So none of them have a codec_tag member or a query_codec
> callback. Personally, I agree that the current method is inelegant but these
> "muxers" are stable and these tests shouldn't need changes. But I can go the
> query_codec route if you advise it.

yes please do
note query_codec is only needed for the few special cases that do more than
one codec_id

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/rawenc.c b/libavformat/rawenc.c
index 809ca23b1a..19028329f7 100644
--- a/libavformat/rawenc.c
+++ b/libavformat/rawenc.c
@@ -34,12 +34,48 @@  int ff_raw_write_packet(AVFormatContext *s, AVPacket *pkt)
 
 static int force_one_stream(AVFormatContext *s)
 {
+enum AVCodecID id;
+const char *idname;
+
     if (s->nb_streams != 1) {
         av_log(s, AV_LOG_ERROR, "%s files have exactly one stream\n",
                s->oformat->name);
         return AVERROR(EINVAL);
     }
+
+    if (!strcmp("data", s->oformat->name))
+        return 0;
+
+id = s->streams[0]->codecpar->codec_id;
+idname = avcodec_get_name(id);
+
+    switch(s->streams[0]->codecpar->codec_type) {
+        case AVMEDIA_TYPE_AUDIO:
+            if (s->oformat->audio_codec != id)
+                goto fail;
+            break;
+        case AVMEDIA_TYPE_VIDEO:
+            if (strstr(s->oformat->name, "mpeg") != NULL) {
+                if (strstr(idname, "mpeg") == NULL)
+                    goto fail;
+            } else if (strstr(s->oformat->name, "m4v") != NULL) {
+                       if (strstr(idname, "mpeg4") == NULL)
+                           goto fail;
+            } else if (strstr(s->oformat->name, "jpeg") != NULL) {
+                       if (strstr(idname, "jpeg") == NULL)
+                           goto fail;
+            } else if (s->oformat->video_codec != id)
+                       goto fail;
+            break;
+        default:
+            goto fail;
+    }
+
     return 0;
+
+fail:
+    av_log(s, AV_LOG_ERROR, "Stream not of type %s\n", s->oformat->name);
+    return AVERROR(EINVAL);
 }
 
 /* Note: Do not forget to add new entries to the Makefile as well. */