[FFmpeg-devel] [PATCH v2 3/3] avformat/dashenc: Set mp4 as the default format for VP9

[Michael Niedermayer]

On Fri, Apr 27, 2018 at 12:35:42AM +0300, Jan Ekström wrote:
> On Thu, Apr 26, 2018 at 12:00 PM, Jeyapal, Karthick <kjeyapal at akamai.com> wrote:
> >
> >
> > On 4/23/18 11:40 AM, Karthick J wrote:
> >> From: Karthick Jeyapal <kjeyapal at akamai.com>
> >>
> >> There is a separate muxer(webmdashenc.c) for supporting VP9+webm output in DASH.
> >> Hence in this muxer we will focus on supporting VP9 in MP4
> >> Have verified playout support of VP9+MP4 in Chrome and Firefox.
> >> ---
> >>  libavformat/dashenc.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> >> index a5f58d4..211ef23 100644
> >> --- a/libavformat/dashenc.c
> >> +++ b/libavformat/dashenc.c
> >> @@ -959,11 +959,10 @@ static int dash_init(AVFormatContext *s)
> >>          if (!ctx)
> >>              return AVERROR(ENOMEM);
> >>
> >> -        // choose muxer based on codec: webm for VP8/9 and opus, mp4 otherwise
> >> +        // choose muxer based on codec: webm for VP8 and opus, mp4 otherwise
> >>          // note: os->format_name is also used as part of the mimetype of the
> >>          //       representation, e.g. video/<format_name>
> >>          if (s->streams[i]->codecpar->codec_id == AV_CODEC_ID_VP8 ||
> >> -            s->streams[i]->codecpar->codec_id == AV_CODEC_ID_VP9 ||
> >>              s->streams[i]->codecpar->codec_id == AV_CODEC_ID_OPUS ||
> >>              s->streams[i]->codecpar->codec_id == AV_CODEC_ID_VORBIS) {
> >>              snprintf(os->format_name, sizeof(os->format_name), "webm");
> >
> > Pushed the patchset
> >
> 
> Hi,
> 
> First of all, I would prefer the patch to actually mention what it is
> doing. It is removing the webm override from the muxer for VP9. There
> is as far as I know no option to switch between modes in current git,
> so the commit message is blindly misleading at best and just plain
> trying to look harmless (in order to get a free pass) if taken the
> wrong way. Not saying you meant it that way, but the commit message
> does not say what it does as far as I can see.
> 
> Also the patch does not mention the reason why it is doing this other
> than the fact that there's a separate webm DASH muxer. That is true,
> but the real reason you were switching this default is because the
> WebM mode does not work. Now, fixing segfaults is good. And, for the
> record, I actually agree with the change since with the profile string
> it works in dash.js on various browsers (Firefox, Chromium, Edge).
> 
> But begesus... If it is done like this you might as well be honest and
> just remove the WebM mode! Because right now you left it there to
> segfault for VP8, Opus, Vorbis. Another alternative would have been to
> apply the small change to Rodger Combs's patch
> (https://patchwork.ffmpeg.org/patch/7984/), which you even commented
> on before! Maybe it still doesn't work in browsers, but at least it
> would have gotten to that point.
> 
> Really, I am thankful that you are contributing, but I really do not
> want to see things like these after long days of work when I have not
> noticed or wasn't able to write a long reply. You waited for two days,
> and pushed without reviews even though I had shown interest in the
> patch in its first iteration. If you are interested in getting quick
> comments from someone (including me when I am awake and available),
> please join the IRC channel #ffmpeg-devel if only possible. Even if it
> is just for pokes and links to patchwork towards someone who has shown
> interest to related patches before. I try as much as possible to poke
> relevant people when I post patches, and I would prefer it if others
> would do that as well. We're not perfect and issues can and do go
> through peoples' eyes (esp. if the change set is of the larger size
> issues tend to get hidden in plain sight, unfortunately), but let's
> try to make this work.
> 
> Best regards,
> Jan
> 
> P.S. I am sorry if my way of speech came out bad, it is just past
> midnight here and I was trying to get a reply to this written after
> noticing this mail.

I can confirm that there still is a segfault occuring
./ffmpeg -i ~/fatesamples/fate/fate-suite/vp8/RRSF49-short.webm -c copy -b:v 2M out.mpd

Jeyapal, do you have time to look at these segfaults?
ffmpeg should not segfault!
Its also very important that no invalid files are generated (just saying so
we dont end with a quick segfault fix that then produces bad files)

Also, does this affect any release branches ?
If so the fixes need to be backported or something else done to make
the releases free of such bugs.

And i have to admit that i did not follow this too closely it was
rather the discussion about it on IRC that pointed me to it.
But if "this" commit was intended to fix the vp9 segfault then its
commit message was incomplete.
And it certainly was confusing multiple people, including me.
Commit messages should clearly describe what is changed, why it
is changed, and so forth. So that others looking at it both
before its pushed on the mailing list as well as potentially long
after understand what was done and why it was done.

Thanks


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180427/d092e790/attachment.sig>