On 4/27/18 3:05 AM, Jan Ekström wrote:
> On Thu, Apr 26, 2018 at 12:00 PM, Jeyapal, Karthick <kjeya...@akamai.com>
> wrote:
>>
>>
>> On 4/23/18 11:40 AM, Karthick J wrote:
>>> From: Karthick Jeyapal <kjeya...@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
