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

[wm4]

On Sat, 14 Jan 2017 06:18:13 +0100
Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:

> 2017-01-13 6:28 GMT+01:00 wm4 <nfxjfg at googlemail.com>:
> > On Thu, 12 Jan 2017 23:31:11 +0100
> > Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
> >  
> >> 2017-01-12 23:20 GMT+01:00 Hendrik Leppkes <h.leppkes at gmail.com>:  
> >> > On Fri, Jan 13, 2017 at 1:10 AM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:  
> >> >> 2017-01-11 12:24 GMT+01:00 Moritz Barsnick <barsnick at 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