diff mbox series

[FFmpeg-devel,v2] movenc: Use first H264/HEVC frame as extradata, if it is missing

Message ID 20200520061021.368-1-martin@martin.st
State Accepted
Headers show
Series [FFmpeg-devel,v2] movenc: Use first H264/HEVC frame as extradata, if it is missing | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Martin Storsjö May 20, 2020, 6:10 a.m. UTC 
Sticking a full frame in the extradata works, as the code for writing
the avcC/hvcC extracts the relevant parameter set NAL units - provided
that they actually exist in the frame.

Some encoders don't provide split out extradata directly on init (or
at all). In particular, the MediaFoundation encoder wrapper doesn't
always (depending on the actual encoder device) - this is the case for
Qualcomm's HEVC encoder on SD835, and also on some QSV H264 encoders).

This only works for cases where the moov hasn't already been written
(e.g. when not writing fragmented mp4 with empty_moov, unless using
the delay_moov option).

Signed-off-by: Martin Storsjö <martin@martin.st>
---
 libavformat/movenc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

James Almer May 20, 2020, 6:20 p.m. UTC | #1 
On 5/20/2020 3:10 AM, Martin Storsjö wrote:
> Sticking a full frame in the extradata works, as the code for writing
> the avcC/hvcC extracts the relevant parameter set NAL units - provided
> that they actually exist in the frame.
> 
> Some encoders don't provide split out extradata directly on init (or
> at all). In particular, the MediaFoundation encoder wrapper doesn't
> always (depending on the actual encoder device) - this is the case for
> Qualcomm's HEVC encoder on SD835, and also on some QSV H264 encoders).
> 
> This only works for cases where the moov hasn't already been written
> (e.g. when not writing fragmented mp4 with empty_moov, unless using
> the delay_moov option).
> 
> Signed-off-by: Martin Storsjö <martin@martin.st>
> ---
>  libavformat/movenc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 27d7621e27..6a85440a3f 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -5584,7 +5584,9 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
>  
>      if ((par->codec_id == AV_CODEC_ID_DNXHD ||
>           par->codec_id == AV_CODEC_ID_TRUEHD ||
> -         par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len) {
> +         par->codec_id == AV_CODEC_ID_AC3 ||
> +         par->codec_id == AV_CODEC_ID_H264 ||
> +         par->codec_id == AV_CODEC_ID_HEVC) && !trk->vos_len) {

nit: You could add the h264 and hevc lines above the truehd one, and get
a smaller diff.

LGTM either way.

>          /* copy frame to create needed atoms */
>          trk->vos_len  = size;
>          trk->vos_data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>
Michael Niedermayer May 21, 2020, 8:03 a.m. UTC | #2 
On Wed, May 20, 2020 at 09:10:21AM +0300, Martin Storsjö wrote:
> Sticking a full frame in the extradata works, as the code for writing
> the avcC/hvcC extracts the relevant parameter set NAL units - provided
> that they actually exist in the frame.
> 
> Some encoders don't provide split out extradata directly on init (or
> at all). In particular, the MediaFoundation encoder wrapper doesn't
> always (depending on the actual encoder device) - this is the case for
> Qualcomm's HEVC encoder on SD835, and also on some QSV H264 encoders).
> 
> This only works for cases where the moov hasn't already been written
> (e.g. when not writing fragmented mp4 with empty_moov, unless using
> the delay_moov option).
> 
> Signed-off-by: Martin Storsjö <martin@martin.st>
> ---
>  libavformat/movenc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index 27d7621e27..6a85440a3f 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -5584,7 +5584,9 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
>  
>      if ((par->codec_id == AV_CODEC_ID_DNXHD ||
>           par->codec_id == AV_CODEC_ID_TRUEHD ||
> -         par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len) {
> +         par->codec_id == AV_CODEC_ID_AC3 ||
> +         par->codec_id == AV_CODEC_ID_H264 ||
> +         par->codec_id == AV_CODEC_ID_HEVC) && !trk->vos_len) {
>          /* copy frame to create needed atoms */
>          trk->vos_len  = size;
>          trk->vos_data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);

This changes avcintra output

example testcase:
./ffmpeg  -i ~/videos/mm-short.mpg -avcintra-class 100 -tune psnr -flags +ildct-global_header -t 0.5 -pix_fmt yuv422p10 -vf scale=1920:1080 -an file.mov


thx

[...]
Martin Storsjö May 21, 2020, 8:10 a.m. UTC | #3 
On Thu, 21 May 2020, Michael Niedermayer wrote:

> On Wed, May 20, 2020 at 09:10:21AM +0300, Martin Storsjö wrote:
>> Sticking a full frame in the extradata works, as the code for writing
>> the avcC/hvcC extracts the relevant parameter set NAL units - provided
>> that they actually exist in the frame.
>>
>> Some encoders don't provide split out extradata directly on init (or
>> at all). In particular, the MediaFoundation encoder wrapper doesn't
>> always (depending on the actual encoder device) - this is the case for
>> Qualcomm's HEVC encoder on SD835, and also on some QSV H264 encoders).
>>
>> This only works for cases where the moov hasn't already been written
>> (e.g. when not writing fragmented mp4 with empty_moov, unless using
>> the delay_moov option).
>>
>> Signed-off-by: Martin Storsjö <martin@martin.st>
>> ---
>>  libavformat/movenc.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> index 27d7621e27..6a85440a3f 100644
>> --- a/libavformat/movenc.c
>> +++ b/libavformat/movenc.c
>> @@ -5584,7 +5584,9 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
>>
>>      if ((par->codec_id == AV_CODEC_ID_DNXHD ||
>>           par->codec_id == AV_CODEC_ID_TRUEHD ||
>> -         par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len) {
>> +         par->codec_id == AV_CODEC_ID_AC3 ||
>> +         par->codec_id == AV_CODEC_ID_H264 ||
>> +         par->codec_id == AV_CODEC_ID_HEVC) && !trk->vos_len) {
>>          /* copy frame to create needed atoms */
>>          trk->vos_len  = size;
>>          trk->vos_data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>
> This changes avcintra output
>
> example testcase:
> ./ffmpeg  -i ~/videos/mm-short.mpg -avcintra-class 100 -tune psnr -flags +ildct-global_header -t 0.5 -pix_fmt yuv422p10 -vf scale=1920:1080 -an file.mov

Right, if explicitly disabling global headers on the encoder, this will 
end up putting them back.

Any ideas on what the best path forward would be?

// Martin
Andreas Rheinhardt May 21, 2020, 9:21 a.m. UTC | #4 
Martin Storsjö:
> On Thu, 21 May 2020, Michael Niedermayer wrote:
> 
>> On Wed, May 20, 2020 at 09:10:21AM +0300, Martin Storsjö wrote:
>>> Sticking a full frame in the extradata works, as the code for writing
>>> the avcC/hvcC extracts the relevant parameter set NAL units - provided
>>> that they actually exist in the frame.
>>>
>>> Some encoders don't provide split out extradata directly on init (or
>>> at all). In particular, the MediaFoundation encoder wrapper doesn't
>>> always (depending on the actual encoder device) - this is the case for
>>> Qualcomm's HEVC encoder on SD835, and also on some QSV H264 encoders).
>>>
>>> This only works for cases where the moov hasn't already been written
>>> (e.g. when not writing fragmented mp4 with empty_moov, unless using
>>> the delay_moov option).
>>>
>>> Signed-off-by: Martin Storsjö <martin@martin.st>
>>> ---
>>>  libavformat/movenc.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>> index 27d7621e27..6a85440a3f 100644
>>> --- a/libavformat/movenc.c
>>> +++ b/libavformat/movenc.c
>>> @@ -5584,7 +5584,9 @@ int ff_mov_write_packet(AVFormatContext *s,
>>> AVPacket *pkt)
>>>
>>>      if ((par->codec_id == AV_CODEC_ID_DNXHD ||
>>>           par->codec_id == AV_CODEC_ID_TRUEHD ||
>>> -         par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len) {
>>> +         par->codec_id == AV_CODEC_ID_AC3 ||
>>> +         par->codec_id == AV_CODEC_ID_H264 ||
>>> +         par->codec_id == AV_CODEC_ID_HEVC) && !trk->vos_len) {
>>>          /* copy frame to create needed atoms */
>>>          trk->vos_len  = size;
>>>          trk->vos_data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>>
>> This changes avcintra output
>>
>> example testcase:
>> ./ffmpeg  -i ~/videos/mm-short.mpg -avcintra-class 100 -tune psnr
>> -flags +ildct-global_header -t 0.5 -pix_fmt yuv422p10 -vf
>> scale=1920:1080 -an file.mov
> 
> Right, if explicitly disabling global headers on the encoder, this will
> end up putting them back.
> 
> Any ideas on what the best path forward would be?
> 
> // Martin

If I am not mistaken, then the sample created by the above command line
is annex b in mp4 which is against the spec. So changing the output is
nothing to worry about.

- Andreas
Martin Storsjö May 21, 2020, 10:19 a.m. UTC | #5 
On Thu, 21 May 2020, Andreas Rheinhardt wrote:

> Martin Storsjö:
>> On Thu, 21 May 2020, Michael Niedermayer wrote:
>> 
>>> On Wed, May 20, 2020 at 09:10:21AM +0300, Martin Storsjö wrote:
>>>> Sticking a full frame in the extradata works, as the code for writing
>>>> the avcC/hvcC extracts the relevant parameter set NAL units - provided
>>>> that they actually exist in the frame.
>>>>
>>>> Some encoders don't provide split out extradata directly on init (or
>>>> at all). In particular, the MediaFoundation encoder wrapper doesn't
>>>> always (depending on the actual encoder device) - this is the case for
>>>> Qualcomm's HEVC encoder on SD835, and also on some QSV H264 encoders).
>>>>
>>>> This only works for cases where the moov hasn't already been written
>>>> (e.g. when not writing fragmented mp4 with empty_moov, unless using
>>>> the delay_moov option).
>>>>
>>>> Signed-off-by: Martin Storsjö <martin@martin.st>
>>>> ---
>>>>  libavformat/movenc.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>> index 27d7621e27..6a85440a3f 100644
>>>> --- a/libavformat/movenc.c
>>>> +++ b/libavformat/movenc.c
>>>> @@ -5584,7 +5584,9 @@ int ff_mov_write_packet(AVFormatContext *s,
>>>> AVPacket *pkt)
>>>>
>>>>      if ((par->codec_id == AV_CODEC_ID_DNXHD ||
>>>>           par->codec_id == AV_CODEC_ID_TRUEHD ||
>>>> -         par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len) {
>>>> +         par->codec_id == AV_CODEC_ID_AC3 ||
>>>> +         par->codec_id == AV_CODEC_ID_H264 ||
>>>> +         par->codec_id == AV_CODEC_ID_HEVC) && !trk->vos_len) {
>>>>          /* copy frame to create needed atoms */
>>>>          trk->vos_len  = size;
>>>>          trk->vos_data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>
>>> This changes avcintra output
>>>
>>> example testcase:
>>> ./ffmpeg  -i ~/videos/mm-short.mpg -avcintra-class 100 -tune psnr
>>> -flags +ildct-global_header -t 0.5 -pix_fmt yuv422p10 -vf
>>> scale=1920:1080 -an file.mov
>> 
>> Right, if explicitly disabling global headers on the encoder, this will
>> end up putting them back.
>> 
>> Any ideas on what the best path forward would be?
>> 
>> // Martin
>
> If I am not mistaken, then the sample created by the above command line
> is annex b in mp4 which is against the spec. So changing the output is
> nothing to worry about.

No, the mov muxer does rewrite the bitstream from annex b to mp4 form, 
regardless of whether there was any broken out parameter sets in the 
extradata.

I presume the intent is that with libx264, normally you'd have parameter 
sets prepended on each IDR frame. If the global headers flag is set, the 
parameter sets are placed in extradata and not prepended on each frame.

And I guess it's needed for the AVC intra spec to have the parameter sets 
included in each frame, instead of tucked away in the avc1 global header.

I guess the main question is, does it matter and/or does it break 
anything, if we now include parameter sets in avc1 like a normal mov/mp4, 
for AVC intra, while the parameter sets still are available within the 
frames as well.

// Martin
Andreas Rheinhardt May 21, 2020, 10:25 a.m. UTC | #6 
Martin Storsjö:
> On Thu, 21 May 2020, Andreas Rheinhardt wrote:
> 
>> Martin Storsjö:
>>> On Thu, 21 May 2020, Michael Niedermayer wrote:
>>>
>>>> On Wed, May 20, 2020 at 09:10:21AM +0300, Martin Storsjö wrote:
>>>>> Sticking a full frame in the extradata works, as the code for writing
>>>>> the avcC/hvcC extracts the relevant parameter set NAL units - provided
>>>>> that they actually exist in the frame.
>>>>>
>>>>> Some encoders don't provide split out extradata directly on init (or
>>>>> at all). In particular, the MediaFoundation encoder wrapper doesn't
>>>>> always (depending on the actual encoder device) - this is the case for
>>>>> Qualcomm's HEVC encoder on SD835, and also on some QSV H264 encoders).
>>>>>
>>>>> This only works for cases where the moov hasn't already been written
>>>>> (e.g. when not writing fragmented mp4 with empty_moov, unless using
>>>>> the delay_moov option).
>>>>>
>>>>> Signed-off-by: Martin Storsjö <martin@martin.st>
>>>>> ---
>>>>>  libavformat/movenc.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>>> index 27d7621e27..6a85440a3f 100644
>>>>> --- a/libavformat/movenc.c
>>>>> +++ b/libavformat/movenc.c
>>>>> @@ -5584,7 +5584,9 @@ int ff_mov_write_packet(AVFormatContext *s,
>>>>> AVPacket *pkt)
>>>>>
>>>>>      if ((par->codec_id == AV_CODEC_ID_DNXHD ||
>>>>>           par->codec_id == AV_CODEC_ID_TRUEHD ||
>>>>> -         par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len) {
>>>>> +         par->codec_id == AV_CODEC_ID_AC3 ||
>>>>> +         par->codec_id == AV_CODEC_ID_H264 ||
>>>>> +         par->codec_id == AV_CODEC_ID_HEVC) && !trk->vos_len) {
>>>>>          /* copy frame to create needed atoms */
>>>>>          trk->vos_len  = size;
>>>>>          trk->vos_data = av_malloc(size +
>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>
>>>> This changes avcintra output
>>>>
>>>> example testcase:
>>>> ./ffmpeg  -i ~/videos/mm-short.mpg -avcintra-class 100 -tune psnr
>>>> -flags +ildct-global_header -t 0.5 -pix_fmt yuv422p10 -vf
>>>> scale=1920:1080 -an file.mov
>>>
>>> Right, if explicitly disabling global headers on the encoder, this will
>>> end up putting them back.
>>>
>>> Any ideas on what the best path forward would be?
>>>
>>> // Martin
>>
>> If I am not mistaken, then the sample created by the above command line
>> is annex b in mp4 which is against the spec. So changing the output is
>> nothing to worry about.
> 
> No, the mov muxer does rewrite the bitstream from annex b to mp4 form,
> regardless of whether there was any broken out parameter sets in the
> extradata.
> 
Sure about that? The check used is

    if (par->codec_id == AV_CODEC_ID_H264 && trk->vos_len > 0 &&
*(uint8_t *)trk->vos_data != 1 && !TAG_IS_AVCI(trk->tag)) {

If trk->vos_len is zero (which is a requirement for your patch to have
any effect), no reformatting will be performed.

- Andreas
Martin Storsjö May 21, 2020, 11 a.m. UTC | #7 
On Thu, 21 May 2020, Andreas Rheinhardt wrote:

> Martin Storsjö:
>> On Thu, 21 May 2020, Andreas Rheinhardt wrote:
>> 
>>> Martin Storsjö:
>>>> On Thu, 21 May 2020, Michael Niedermayer wrote:
>>>>
>>>>> On Wed, May 20, 2020 at 09:10:21AM +0300, Martin Storsjö wrote:
>>>>>> Sticking a full frame in the extradata works, as the code for writing
>>>>>> the avcC/hvcC extracts the relevant parameter set NAL units - provided
>>>>>> that they actually exist in the frame.
>>>>>>
>>>>>> Some encoders don't provide split out extradata directly on init (or
>>>>>> at all). In particular, the MediaFoundation encoder wrapper doesn't
>>>>>> always (depending on the actual encoder device) - this is the case for
>>>>>> Qualcomm's HEVC encoder on SD835, and also on some QSV H264 encoders).
>>>>>>
>>>>>> This only works for cases where the moov hasn't already been written
>>>>>> (e.g. when not writing fragmented mp4 with empty_moov, unless using
>>>>>> the delay_moov option).
>>>>>>
>>>>>> Signed-off-by: Martin Storsjö <martin@martin.st>
>>>>>> ---
>>>>>>  libavformat/movenc.c | 4 +++-
>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>>>>> index 27d7621e27..6a85440a3f 100644
>>>>>> --- a/libavformat/movenc.c
>>>>>> +++ b/libavformat/movenc.c
>>>>>> @@ -5584,7 +5584,9 @@ int ff_mov_write_packet(AVFormatContext *s,
>>>>>> AVPacket *pkt)
>>>>>>
>>>>>>      if ((par->codec_id == AV_CODEC_ID_DNXHD ||
>>>>>>           par->codec_id == AV_CODEC_ID_TRUEHD ||
>>>>>> -         par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len) {
>>>>>> +         par->codec_id == AV_CODEC_ID_AC3 ||
>>>>>> +         par->codec_id == AV_CODEC_ID_H264 ||
>>>>>> +         par->codec_id == AV_CODEC_ID_HEVC) && !trk->vos_len) {
>>>>>>          /* copy frame to create needed atoms */
>>>>>>          trk->vos_len  = size;
>>>>>>          trk->vos_data = av_malloc(size +
>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>
>>>>> This changes avcintra output
>>>>>
>>>>> example testcase:
>>>>> ./ffmpeg  -i ~/videos/mm-short.mpg -avcintra-class 100 -tune psnr
>>>>> -flags +ildct-global_header -t 0.5 -pix_fmt yuv422p10 -vf
>>>>> scale=1920:1080 -an file.mov
>>>>
>>>> Right, if explicitly disabling global headers on the encoder, this will
>>>> end up putting them back.
>>>>
>>>> Any ideas on what the best path forward would be?
>>>>
>>>> // Martin
>>>
>>> If I am not mistaken, then the sample created by the above command line
>>> is annex b in mp4 which is against the spec. So changing the output is
>>> nothing to worry about.
>> 
>> No, the mov muxer does rewrite the bitstream from annex b to mp4 form,
>> regardless of whether there was any broken out parameter sets in the
>> extradata.
>> 
> Sure about that? The check used is
>
>    if (par->codec_id == AV_CODEC_ID_H264 && trk->vos_len > 0 &&
> *(uint8_t *)trk->vos_data != 1 && !TAG_IS_AVCI(trk->tag)) {
>
> If trk->vos_len is zero (which is a requirement for your patch to have
> any effect), no reformatting will be performed.

Oh, indeed, you're right.

However wrong that is from the mov/mp4 point of view, it may very well be 
that AVC intra is supposed to be written like this in mov/mp4 files - but 
we'd need a word from somebody who actually knows that case.

Hmm, and unfortunately, this check is further up in the function, than 
when creating the vos_data/vos_len from the frame, so the first frame 
doesn't end up converted correctly in my case... Will send a new patch to 
fix that.

// Martin
Jan Ekström May 21, 2020, 11:32 a.m. UTC | #8 
On Thu, May 21, 2020 at 2:00 PM Martin Storsjö <martin@martin.st> wrote:
>
> Oh, indeed, you're right.
>
> However wrong that is from the mov/mp4 point of view, it may very well be
> that AVC intra is supposed to be written like this in mov/mp4 files - but
> we'd need a word from somebody who actually knows that case.
>

The best way to figure out if a user wants to do AVC Intra muxing
would be to either a) require specific codec_tag/muxing mode be set by
user or b) attempt to somehow properly check the actual bit stream
within `mov_get_h264_codec_tag` to make the selection proper. Right
now the system seems to base that if you are doing H.264, and have no
codec_tag set already, then `mov_get_h264_codec_tag` attempts to set
an AVC Intra codec_tag (which hits TAG_IS_AVCI), which then attempts
to mux things in that proprietary way. Currently the check only checks
for 10bit 4:2:0/4:2:2 or specific resolutions (which include 1080p and
such). And I'm not sure if even the fallback there is a standard
codec_tag, since it is not one of those I remember from 14496-15
(although my memory might just be bad).

By default, and if the output got written with a standard 14496-15 etc
codec_tag, then AVC intra muxing mode should not utilized.

And just a note for the record, the logic in `mov_get_h264_codec_tag`
seems to already be somewhat imperfect, as I've seen users attempting
to encode 10bit H.264 normally into MOV, and one had to force a
codec_tag to be avc1 for a non-broken file to be generated (as in,
even FFmpeg itself couldn't read the file after muxing with the
default selected codec_tag).

Just my two cents,
Jan
Martin Storsjö May 21, 2020, 12:12 p.m. UTC | #9 
On Thu, 21 May 2020, Jan Ekström wrote:

> On Thu, May 21, 2020 at 2:00 PM Martin Storsjö <martin@martin.st> wrote:
>>
>> Oh, indeed, you're right.
>>
>> However wrong that is from the mov/mp4 point of view, it may very well be
>> that AVC intra is supposed to be written like this in mov/mp4 files - but
>> we'd need a word from somebody who actually knows that case.
>>
>
> The best way to figure out if a user wants to do AVC Intra muxing
> would be to either a) require specific codec_tag/muxing mode be set by
> user or b) attempt to somehow properly check the actual bit stream
> within `mov_get_h264_codec_tag` to make the selection proper. Right
> now the system seems to base that if you are doing H.264, and have no
> codec_tag set already, then `mov_get_h264_codec_tag` attempts to set
> an AVC Intra codec_tag (which hits TAG_IS_AVCI), which then attempts
> to mux things in that proprietary way.

Thanks for the insight into this case, that it really is intended to mux 
it this way.

I hadn't known what TAG_IS_AVCI is really, but with that it should be easy 
to fix this case.

// Martin
Martin Storsjö May 21, 2020, 12:16 p.m. UTC | #10 
On Thu, 21 May 2020, Michael Niedermayer wrote:

> On Wed, May 20, 2020 at 09:10:21AM +0300, Martin Storsjö wrote:
>> Sticking a full frame in the extradata works, as the code for writing
>> the avcC/hvcC extracts the relevant parameter set NAL units - provided
>> that they actually exist in the frame.
>>
>> Some encoders don't provide split out extradata directly on init (or
>> at all). In particular, the MediaFoundation encoder wrapper doesn't
>> always (depending on the actual encoder device) - this is the case for
>> Qualcomm's HEVC encoder on SD835, and also on some QSV H264 encoders).
>>
>> This only works for cases where the moov hasn't already been written
>> (e.g. when not writing fragmented mp4 with empty_moov, unless using
>> the delay_moov option).
>>
>> Signed-off-by: Martin Storsjö <martin@martin.st>
>> ---
>>  libavformat/movenc.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> index 27d7621e27..6a85440a3f 100644
>> --- a/libavformat/movenc.c
>> +++ b/libavformat/movenc.c
>> @@ -5584,7 +5584,9 @@ int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
>>
>>      if ((par->codec_id == AV_CODEC_ID_DNXHD ||
>>           par->codec_id == AV_CODEC_ID_TRUEHD ||
>> -         par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len) {
>> +         par->codec_id == AV_CODEC_ID_AC3 ||
>> +         par->codec_id == AV_CODEC_ID_H264 ||
>> +         par->codec_id == AV_CODEC_ID_HEVC) && !trk->vos_len) {
>>          /* copy frame to create needed atoms */
>>          trk->vos_len  = size;
>>          trk->vos_data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>
> This changes avcintra output
>
> example testcase:
> ./ffmpeg  -i ~/videos/mm-short.mpg -avcintra-class 100 -tune psnr -flags +ildct-global_header -t 0.5 -pix_fmt yuv422p10 -vf scale=1920:1080 -an file.mov

I believe this should be fixed by "[PATCH v2] movenc: Fix conversion of 
the first frame for extradata-less H264/HEVC".

// Martin
diff mbox series

Patch

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 27d7621e27..6a85440a3f 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -5584,7 +5584,9 @@  int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
 
     if ((par->codec_id == AV_CODEC_ID_DNXHD ||
          par->codec_id == AV_CODEC_ID_TRUEHD ||
-         par->codec_id == AV_CODEC_ID_AC3) && !trk->vos_len) {
+         par->codec_id == AV_CODEC_ID_AC3 ||
+         par->codec_id == AV_CODEC_ID_H264 ||
+         par->codec_id == AV_CODEC_ID_HEVC) && !trk->vos_len) {
         /* copy frame to create needed atoms */
         trk->vos_len  = size;
         trk->vos_data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);