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

[Jan Ekström]

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.