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

[Michael Niedermayer]

On Fri, Apr 27, 2018 at 04:33:27AM +0000, Jeyapal, Karthick wrote:
> 
> 
> On 4/27/18 4:26 AM, Michael Niedermayer wrote:
> > 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)
> We do have a fix from Jan for segfault issue. http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/229082.html
> I will test it and push it after 3 days. 

> I will think it affects 4.0 branch as well. Should I push that patch to 4.0 as well?

yes if you are sure that its better with the patch for 4.0

thanks

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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/bc6b6834/attachment.sig>