[FFmpeg-devel] lavf/matroskaenc: Print an error if an unreadable rawvideo pix_fmt is written

Submitted by Carl Eugen Hoyos on Jan. 10, 2017, 4:05 p.m.

Details

Message ID 201701101705.47583.cehoyos@ag.or.at
State New
Headers show

Commit Message

Carl Eugen Hoyos Jan. 10, 2017, 4:05 p.m.
Hi!

Attached patch copies mov and avi behaviour.

Please comment, Carl Eugen
From c716fb256cfcc59dfa6adc93ac0810db9c2c1722 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <cehoyos@ag.or.at>
Date: Tue, 10 Jan 2017 17:04:34 +0100
Subject: [PATCH] lavf/matroskaenc: Print an error if rawvideo pix_fmt is
 invalid for vfw.

---
 libavformat/matroskaenc.c |    7 +++++++
 1 file changed, 7 insertions(+)

Comments

Moritz Barsnick Jan. 11, 2017, 11:24 a.m.
On Tue, Jan 10, 2017 at 17:05:47 +0100, Carl Eugen Hoyos wrote:
> +                enum AVPixelFormat pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
> +                                                                 par->bits_per_coded_sample);
> +                if (par->format != pix_fmt && par->format != AV_PIX_FMT_NONE)
> +                    av_log(s, AV_LOG_ERROR, "%s rawvideo cannot be written to vfw mkv, output file will be unreadable\n",
> +                          av_get_pix_fmt_name(par->format));

Should it really be an error (and not a warning) if its detection
doesn't change ffmpeg's behavior?

Just wondering,
Moritz
Carl Eugen Hoyos Jan. 12, 2017, 2:10 p.m.
2017-01-11 12:24 GMT+01:00 Moritz Barsnick <barsnick@gmx.net>:
> On Tue, Jan 10, 2017 at 17:05:47 +0100, Carl Eugen Hoyos wrote:
>> +                enum AVPixelFormat pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
>> +                                                                 par->bits_per_coded_sample);
>> +                if (par->format != pix_fmt && par->format != AV_PIX_FMT_NONE)
>> +                    av_log(s, AV_LOG_ERROR, "%s rawvideo cannot be written to vfw mkv, output file will be unreadable\n",
>> +                          av_get_pix_fmt_name(par->format));
>
> Should it really be an error (and not a warning) if its detection
> doesn't change ffmpeg's behavior?

Just copying what avi and mov muxer do.

Note that (many) decoders print errors and (can) continue.

(I believe library users can - and do - put rawvideo into standard
containers and force the right pix_fmt on reading.)

Carl Eugen
Hendrik Leppkes Jan. 12, 2017, 10:20 p.m.
On Fri, Jan 13, 2017 at 1:10 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2017-01-11 12:24 GMT+01:00 Moritz Barsnick <barsnick@gmx.net>:
>> On Tue, Jan 10, 2017 at 17:05:47 +0100, Carl Eugen Hoyos wrote:
>>> +                enum AVPixelFormat pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
>>> +                                                                 par->bits_per_coded_sample);
>>> +                if (par->format != pix_fmt && par->format != AV_PIX_FMT_NONE)
>>> +                    av_log(s, AV_LOG_ERROR, "%s rawvideo cannot be written to vfw mkv, output file will be unreadable\n",
>>> +                          av_get_pix_fmt_name(par->format));
>>
>> Should it really be an error (and not a warning) if its detection
>> doesn't change ffmpeg's behavior?
>
> Just copying what avi and mov muxer do.
>
> Note that (many) decoders print errors and (can) continue.
>
> (I believe library users can - and do - put rawvideo into standard
> containers and force the right pix_fmt on reading.)
>

That seems silly, we shouldn't log an error that a file will be broken
and then still write it. Muxers should be strict and write only valid
files.

- Hendrik
Carl Eugen Hoyos Jan. 12, 2017, 10:31 p.m.
2017-01-12 23:20 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:
> On Fri, Jan 13, 2017 at 1:10 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> 2017-01-11 12:24 GMT+01:00 Moritz Barsnick <barsnick@gmx.net>:
>>> On Tue, Jan 10, 2017 at 17:05:47 +0100, Carl Eugen Hoyos wrote:
>>>> +                enum AVPixelFormat pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
>>>> +                                                                 par->bits_per_coded_sample);
>>>> +                if (par->format != pix_fmt && par->format != AV_PIX_FMT_NONE)
>>>> +                    av_log(s, AV_LOG_ERROR, "%s rawvideo cannot be written to vfw mkv, output file will be unreadable\n",
>>>> +                          av_get_pix_fmt_name(par->format));
>>>
>>> Should it really be an error (and not a warning) if its detection
>>> doesn't change ffmpeg's behavior?
>>
>> Just copying what avi and mov muxer do.
>>
>> Note that (many) decoders print errors and (can) continue.
>>
>> (I believe library users can - and do - put rawvideo into standard
>> containers and force the right pix_fmt on reading.)
>
> That seems silly, we shouldn't log an error that a file will be broken
> and then still write it. Muxers should be strict and write only valid
> files.

This is what we currently do for both other multi-purpose containers
(while FFmpeg logs no message for unreadable transport streams),
so lets please commit this.

Carl Eugen
wm4 Jan. 13, 2017, 5:28 a.m.
On Thu, 12 Jan 2017 23:31:11 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-12 23:20 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:
> > On Fri, Jan 13, 2017 at 1:10 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:  
> >> 2017-01-11 12:24 GMT+01:00 Moritz Barsnick <barsnick@gmx.net>:  
> >>> On Tue, Jan 10, 2017 at 17:05:47 +0100, Carl Eugen Hoyos wrote:  
> >>>> +                enum AVPixelFormat pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
> >>>> +                                                                 par->bits_per_coded_sample);
> >>>> +                if (par->format != pix_fmt && par->format != AV_PIX_FMT_NONE)
> >>>> +                    av_log(s, AV_LOG_ERROR, "%s rawvideo cannot be written to vfw mkv, output file will be unreadable\n",
> >>>> +                          av_get_pix_fmt_name(par->format));  
> >>>
> >>> Should it really be an error (and not a warning) if its detection
> >>> doesn't change ffmpeg's behavior?  
> >>
> >> Just copying what avi and mov muxer do.
> >>
> >> Note that (many) decoders print errors and (can) continue.
> >>
> >> (I believe library users can - and do - put rawvideo into standard
> >> containers and force the right pix_fmt on reading.)  
> >
> > That seems silly, we shouldn't log an error that a file will be broken
> > and then still write it. Muxers should be strict and write only valid
> > files.  
> 
> This is what we currently do for both other multi-purpose containers
> (while FFmpeg logs no message for unreadable transport streams),
> so lets please commit this.

I have to agree with Hendrik. Your patch is all about adding a message
for this case. Why not do the right thing in the first place? You'd
only have to add an error return or so.
Carl Eugen Hoyos Jan. 14, 2017, 5:18 a.m.
2017-01-13 6:28 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Thu, 12 Jan 2017 23:31:11 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-01-12 23:20 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:
>> > On Fri, Jan 13, 2017 at 1:10 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >> 2017-01-11 12:24 GMT+01:00 Moritz Barsnick <barsnick@gmx.net>:
>> >>> On Tue, Jan 10, 2017 at 17:05:47 +0100, Carl Eugen Hoyos wrote:
>> >>>> +                enum AVPixelFormat pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
>> >>>> +                                                                 par->bits_per_coded_sample);
>> >>>> +                if (par->format != pix_fmt && par->format != AV_PIX_FMT_NONE)
>> >>>> +                    av_log(s, AV_LOG_ERROR, "%s rawvideo cannot be written to vfw mkv, output file will be unreadable\n",
>> >>>> +                          av_get_pix_fmt_name(par->format));
>> >>>
>> >>> Should it really be an error (and not a warning) if its detection
>> >>> doesn't change ffmpeg's behavior?
>> >>
>> >> Just copying what avi and mov muxer do.
>> >>
>> >> Note that (many) decoders print errors and (can) continue.
>> >>
>> >> (I believe library users can - and do - put rawvideo into standard
>> >> containers and force the right pix_fmt on reading.)
>> >
>> > That seems silly, we shouldn't log an error that a file will be broken
>> > and then still write it. Muxers should be strict and write only valid
>> > files.
>>
>> This is what we currently do for both other multi-purpose containers
>> (while FFmpeg logs no message for unreadable transport streams),
>> so lets please commit this.
>
> I have to agree with Hendrik. Your patch is all about adding a message
> for this case.

The same message that gets printed for the other general-purpose
muxers.

> Why not do the right thing in the first place?

I believe informing users is the right thing.

> You'd only have to add an error return or so.

I would prefer not to harm library users when helping cli users.

Carl Eugen
Hendrik Leppkes Jan. 14, 2017, 6:04 a.m.
On Sat, Jan 14, 2017 at 4:18 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2017-01-13 6:28 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> On Thu, 12 Jan 2017 23:31:11 +0100
>> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>
>>> 2017-01-12 23:20 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:
>>> > On Fri, Jan 13, 2017 at 1:10 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>> >> 2017-01-11 12:24 GMT+01:00 Moritz Barsnick <barsnick@gmx.net>:
>>> >>> On Tue, Jan 10, 2017 at 17:05:47 +0100, Carl Eugen Hoyos wrote:
>>> >>>> +                enum AVPixelFormat pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
>>> >>>> +                                                                 par->bits_per_coded_sample);
>>> >>>> +                if (par->format != pix_fmt && par->format != AV_PIX_FMT_NONE)
>>> >>>> +                    av_log(s, AV_LOG_ERROR, "%s rawvideo cannot be written to vfw mkv, output file will be unreadable\n",
>>> >>>> +                          av_get_pix_fmt_name(par->format));
>>> >>>
>>> >>> Should it really be an error (and not a warning) if its detection
>>> >>> doesn't change ffmpeg's behavior?
>>> >>
>>> >> Just copying what avi and mov muxer do.
>>> >>
>>> >> Note that (many) decoders print errors and (can) continue.
>>> >>
>>> >> (I believe library users can - and do - put rawvideo into standard
>>> >> containers and force the right pix_fmt on reading.)
>>> >
>>> > That seems silly, we shouldn't log an error that a file will be broken
>>> > and then still write it. Muxers should be strict and write only valid
>>> > files.
>>>
>>> This is what we currently do for both other multi-purpose containers
>>> (while FFmpeg logs no message for unreadable transport streams),
>>> so lets please commit this.
>>
>> I have to agree with Hendrik. Your patch is all about adding a message
>> for this case.
>
> The same message that gets printed for the other general-purpose
> muxers.
>
>> Why not do the right thing in the first place?
>
> I believe informing users is the right thing.
>
>> You'd only have to add an error return or so.
>
> I would prefer not to harm library users when helping cli users.
>

Protection the world from writing broken unreadable files doesn't harm
anyone either.

- Hendrik
wm4 Jan. 14, 2017, 11:54 a.m.
On Sat, 14 Jan 2017 06:18:13 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-13 6:28 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Thu, 12 Jan 2017 23:31:11 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-01-12 23:20 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:  
> >> > On Fri, Jan 13, 2017 at 1:10 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:  
> >> >> 2017-01-11 12:24 GMT+01:00 Moritz Barsnick <barsnick@gmx.net>:  
> >> >>> On Tue, Jan 10, 2017 at 17:05:47 +0100, Carl Eugen Hoyos wrote:  
> >> >>>> +                enum AVPixelFormat pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
> >> >>>> +                                                                 par->bits_per_coded_sample);
> >> >>>> +                if (par->format != pix_fmt && par->format != AV_PIX_FMT_NONE)
> >> >>>> +                    av_log(s, AV_LOG_ERROR, "%s rawvideo cannot be written to vfw mkv, output file will be unreadable\n",
> >> >>>> +                          av_get_pix_fmt_name(par->format));  
> >> >>>
> >> >>> Should it really be an error (and not a warning) if its detection
> >> >>> doesn't change ffmpeg's behavior?  
> >> >>
> >> >> Just copying what avi and mov muxer do.
> >> >>
> >> >> Note that (many) decoders print errors and (can) continue.
> >> >>
> >> >> (I believe library users can - and do - put rawvideo into standard
> >> >> containers and force the right pix_fmt on reading.)  
> >> >
> >> > That seems silly, we shouldn't log an error that a file will be broken
> >> > and then still write it. Muxers should be strict and write only valid
> >> > files.  
> >>
> >> This is what we currently do for both other multi-purpose containers
> >> (while FFmpeg logs no message for unreadable transport streams),
> >> so lets please commit this.  
> >
> > I have to agree with Hendrik. Your patch is all about adding a message
> > for this case.  
> 
> The same message that gets printed for the other general-purpose
> muxers.
> 
> > Why not do the right thing in the first place?  
> 
> I believe informing users is the right thing.
> 
> > You'd only have to add an error return or so.  
> 
> I would prefer not to harm library users when helping cli users.

In other places we make an effort to not write potentially broken files
by default, such as adding support for unfinished specifications. Why
write broken files in this specific case, even though we definitely
know it's broken, to the extent of adding a message informing the user
about it.

I'm saying that

1. It's definitely preferable not to write broken files by default if
   we can easily prevent it (i.e. it's "the right thing")

2. A message won't prevent any harm - users might not read it, or the
   encoder is not ffmpeg.c but someone using the API (possibly never
   showing the log messages to the user)

3. There is no argument about consistency, because it's already
   inconsistent, and consistently writing broken files by default is
   not a worthy goal

4. It's trivial to do the right thing

Patch hide | download patch | download mbox

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 78a621e..3f06faa 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -51,10 +51,12 @@ 
 #include "libavutil/samplefmt.h"
 #include "libavutil/sha.h"
 #include "libavutil/stereo3d.h"
+#include "libavutil/pixdesc.h"
 
 #include "libavcodec/xiph.h"
 #include "libavcodec/mpeg4audio.h"
 #include "libavcodec/internal.h"
+#include "libavcodec/raw.h"
 
 typedef struct ebml_master {
     int64_t         pos;                ///< absolute offset in the file where the master's elements start
@@ -1128,6 +1130,11 @@  static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
         }
         if (par->codec_id == AV_CODEC_ID_RAWVIDEO && !par->codec_tag) {
             if (mkv->allow_raw_vfw) {
+                enum AVPixelFormat pix_fmt = avpriv_find_pix_fmt(avpriv_pix_fmt_bps_avi,
+                                                                 par->bits_per_coded_sample);
+                if (par->format != pix_fmt && par->format != AV_PIX_FMT_NONE)
+                    av_log(s, AV_LOG_ERROR, "%s rawvideo cannot be written to vfw mkv, output file will be unreadable\n",
+                          av_get_pix_fmt_name(par->format));
                 native_id = 0;
             } else {
                 av_log(s, AV_LOG_ERROR, "Raw RGB is not supported Natively in Matroska, you can use AVI or NUT or\n"