| Message ID | 201701101705.47583.cehoyos@ag.or.at |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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.
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
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
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
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"
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(+)