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

[Jeyapal, Karthick]

On 4/27/18 3:05 AM, 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.
I am sorry, if the commit comments are misleading and wrong. Yes, I agree that this patch was meant to solve the segfault issue but didn't mention it forthright in the commit message.(which I will correct next time). But I still think that this is the right approach to fix that problem. I had provided the justifications for this approach in my earlier reply http://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228479.html . Since you didn't reply for 1 week, I assumed that you are convinced with the justifications and pushed this patch. Just to re-iterate the justifications,
1. Solving segfault with VP9+webm didn't get the playout working in browsers. 
2. There is a separate webm DASH muxer meant for this exact purpose. Repeating the same here is a waste of time and effort.
3. There was no muxer to support VP9+MP4. So, this patch is also a feature addition. And MP4 is a more widely supported than WebM.
4. Removing WebM support immediately in a single commit is little drastic. I don't know if someone wants WebM with dashenc.c instead of webmdashenc.c for some specific reason. Hence, I wanted to give some time, before removing it completely.
I was thinking to move VP8, Opus and Vorbis to MP4 after this patch gets pushed, to solve this segfault issue for other codecs. 
>
> 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.
Yes, I understand your concerns and I would also definitely want to make this work. To be fair practically I waited for 7 days for your reply before pushing this patch (technically you could argue that it is just 2 or 3 days since V2 patchset was submitted. But you do know that this patch didn't change a character between V1 and V2). But next time around I will make sure that I ping you (or the relevant people) before pushing the patches that you(or they) have shown interest. 
>
> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel