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

Jeyapal, Karthick kjeyapal at akamai.com
Fri Apr 27 07:33:27 EEST 2018



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?
>
> 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.
As I had mentioned in the other thread, I am sorry about it. It was my mistake. Next time around I will provide complete details in the commit message.
>
> Thanks
>
>
> [...]




More information about the ffmpeg-devel mailing list