diff mbox

[FFmpeg-devel] avformat/movenc: Always look up mov tag for AV1.

Message ID 20180813222700.4132-1-bztdlinux@gmail.com
State New
Headers show

Commit Message

Thomas Daede Aug. 13, 2018, 10:27 p.m. UTC
Some other containers for AV1, in particular ivf, use a different
capitalization of the AV1 codec tag. When these files are remuxed
into mp4, they result in invalid files.

This patch normalizes the tag by forcing its lookup in
ff_codec_movvideo_tags.
---
 libavformat/movenc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Carl Eugen Hoyos Aug. 13, 2018, 10:30 p.m. UTC | #1
2018-08-14 0:27 GMT+02:00, Thomas Daede <bztdlinux@gmail.com>:
> Some other containers for AV1, in particular ivf, use a different
> capitalization of the AV1 codec tag. When these files are remuxed
> into mp4, they result in invalid files.
>
> This patch normalizes the tag by forcing its lookup in
> ff_codec_movvideo_tags.

Why isn't av1 added like the existing codecs?

Carl Eugen
Thomas Daede Aug. 13, 2018, 10:36 p.m. UTC | #2
On 08/13/2018 03:30 PM, Carl Eugen Hoyos wrote:
> Why isn't av1 added like the existing codecs?

That would generate invalid AV1 files if -strict -1 or -2 specified (and
currently AV1 mov writing is behind -2). Actually, I'm not sure why the
other codecs are behind the compliance flag either - I'm not really sure
what the use case for those is.
Carl Eugen Hoyos Aug. 13, 2018, 10:47 p.m. UTC | #3
2018-08-14 0:36 GMT+02:00, Thomas Daede <bztdlinux@gmail.com>:
> On 08/13/2018 03:30 PM, Carl Eugen Hoyos wrote:
>> Why isn't av1 added like the existing codecs?
>
> That would generate invalid AV1 files if -strict -1 or -2 specified

That is the idea of -1 and -2.

> (and currently AV1 mov writing is behind -2).

Is the specification still unfinished?
Then what is the exact purpose of this patch?

Carl Eugen
James Almer Aug. 13, 2018, 10:58 p.m. UTC | #4
On 8/13/2018 7:47 PM, Carl Eugen Hoyos wrote:
> 2018-08-14 0:36 GMT+02:00, Thomas Daede <bztdlinux@gmail.com>:
>> On 08/13/2018 03:30 PM, Carl Eugen Hoyos wrote:
>>> Why isn't av1 added like the existing codecs?
>>
>> That would generate invalid AV1 files if -strict -1 or -2 specified
> 
> That is the idea of -1 and -2.
> 
>> (and currently AV1 mov writing is behind -2).
> 
> Is the specification still unfinished?

Yes.

> Then what is the exact purpose of this patch?

The issue at hand is that when remuxing from ivf, the propagated codec
tag AV01 is written as is instead of the correct av01 (as listed in
ff_codec_movvideo_tags and codec_mp4_tags) because a case insensitive
check takes place in either ffmpeg.c or libavformat generic code (i
don't recall exactly where), which of course assumes av01 == AV01.
The latter is only correct for ivf, not mp4.

If this patch adds the AV1 codec id inside the "s->strict_std_compliance
>= FF_COMPLIANCE_NORMAL" check as you suggest, and a muxing process is
done with -strict experimental, then the wrong codec_tag will be written.

An alternative would be to change the aforementioned case insensitive
check into a case sensitive one, but that could break some other
scenarios as i presume it was made that way for a reason.
Thomas Daede Aug. 13, 2018, 11 p.m. UTC | #5
On 08/13/2018 03:47 PM, Carl Eugen Hoyos wrote:
> 2018-08-14 0:36 GMT+02:00, Thomas Daede <bztdlinux@gmail.com>:
>> On 08/13/2018 03:30 PM, Carl Eugen Hoyos wrote:
>>> Why isn't av1 added like the existing codecs?
>>
>> That would generate invalid AV1 files if -strict -1 or -2 specified
> 
> That is the idea of -1 and -2.

The idea of -1 and -2 is to allow muxing in additional nonstandard ways.
However, putting this particular check behind the compliance condition
would change a working muxer to a broken one once you enabled
experimental, which I don't think is desirable.

Presumably the check for the other codecs is to allow proprietary
fourccs to be used when muxing them. I'm not sure when this is desirable
for those codecs, but it certainly isn't for AV1 as it doesn't have any
proprietary fourccs.

>Is the specification still unfinished?
>Then what is the exact purpose of this patch?

It is unfinished, but this patch ffmpeg's implementation closer to the
current draft.
James Almer Aug. 13, 2018, 11:02 p.m. UTC | #6
On 8/13/2018 7:58 PM, James Almer wrote:
> On 8/13/2018 7:47 PM, Carl Eugen Hoyos wrote:
>> 2018-08-14 0:36 GMT+02:00, Thomas Daede <bztdlinux@gmail.com>:
>>> On 08/13/2018 03:30 PM, Carl Eugen Hoyos wrote:
>>>> Why isn't av1 added like the existing codecs?
>>>
>>> That would generate invalid AV1 files if -strict -1 or -2 specified
>>
>> That is the idea of -1 and -2.
>>
>>> (and currently AV1 mov writing is behind -2).
>>
>> Is the specification still unfinished?
> 
> Yes.
> 
>> Then what is the exact purpose of this patch?
> 
> The issue at hand is that when remuxing from ivf, the propagated codec
> tag AV01 is written as is instead of the correct av01 (as listed in
> ff_codec_movvideo_tags and codec_mp4_tags) because a case insensitive
> check takes place in either ffmpeg.c or libavformat generic code (i
> don't recall exactly where), which of course assumes av01 == AV01.
> The latter is only correct for ivf, not mp4.
> 
> If this patch adds the AV1 codec id inside the "s->strict_std_compliance
>> = FF_COMPLIANCE_NORMAL" check as you suggest, and a muxing process is
> done with -strict experimental, then the wrong codec_tag will be written.
> 
> An alternative would be to change the aforementioned case insensitive
> check into a case sensitive one, but that could break some other
> scenarios as i presume it was made that way for a reason.

If you're curious why this doesn't happen with VP9, another format
supported by both ivf and mp4, it's because ivf is VP90 and mp4 is vp09.
diff mbox

Patch

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index d530f40cab..ff36e1376c 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -1542,7 +1542,8 @@  static int mov_get_codec_tag(AVFormatContext *s, MOVTrack *track)
 {
     int tag = track->par->codec_tag;
 
-    if (!tag || (s->strict_std_compliance >= FF_COMPLIANCE_NORMAL &&
+    if (!tag || track->par->codec_id == AV_CODEC_ID_AV1 ||
+                (s->strict_std_compliance >= FF_COMPLIANCE_NORMAL &&
                  (track->par->codec_id == AV_CODEC_ID_DVVIDEO ||
                   track->par->codec_id == AV_CODEC_ID_RAWVIDEO ||
                   track->par->codec_id == AV_CODEC_ID_H263 ||