[Ffmpeg-devel] Re: [PATCH] x264 avc encoding, movenc avcC, ctts

[Baptiste COUDURIER]

Luca Abeni wrote:
> Hi Baptiste,
> 
> I do not know anything about .mov files and about the MOV muxer, but I
> think the "codec" part of your patches (x264_avc_nal_encode.patch) might
> be incorrect:
> 
> On Sun, 2006-02-19 at 18:23 +0100, Baptiste COUDURIER wrote:
> [...]
> 
>>x264_avc_nal_encode.patch
>>
>>which encodes nals correctly for AVC and write extradata for avcC atom.
> 
> BTW, what is AVC? I was under the impression that AVC (Advanced Video
> Coding) = MPEG4 Part 10 = H.264... So, I cannot understand the "encode
> nals correctly for AVC" part
> [...] 

AVC is special variant of H264 found in mp4/mov containers. In that
special variant, nals are encoded a different way. Current h264 decoder
implement both implementations. You can see in the code lines like :
if (h->is_avc).

> [...]
> 
> This breaks applications using extradata. Why do you need to change the
> extradata format? I am not really an expert, but I think extradata
> should be the "global headers", and according to some drafts I read the
> current content of extradata correctly matches H.264 global headers.

I agree, I can set something like if (x4->is_avc). But anyway, extradata
in mp4/mov needs to be formatted the way I patched it.

> Also, maybe I am wrong, but I was under the impression that
> CODEC_FLAG_GLOBAL_HEADER should not change the format of the stream, but
> should simply move "out of band" some parts of the stream (the global
> headers, in this case). Is this wrong? Can someone of the ffmpeg
> developers clarify this?

In fact, when AAC is put in MP4/MOV container it does not contain ADTS
header, while in AVI it contains them. Same for H264, when in AVI, long
nal header is used, while in MP4/MOV, nal size and no header/syncword.

> [...]
> 
>>+        /* pps */
>>+        p += 2; /* advance to stock size */
>>+        s -= 2;
>>+        size = x264_nal_encode(p, &s, 0, &nal[2]);
>>+        if (size < 0)
>>+            return -1;
>>+        /* write size */
>>+        p -= 2;
>>+        size_be = BE_16(&size);
> 
> In any case, if the patch is committed this "p += 2; .... p -= 2;" thing
> should be avoided (maybe something like
> x264_nal_encode(p+2, &s, 0, &nal[2]);... memcpy(p, &size_be, 2); p += size + 2;...)

I agree, I asked for some advices when I submitted, Im not that skilled
and I miss some coding ideas. I will clean today, but if you have ideas,
I would be happy to hear them.

> Moreover, I think you are opencoding the for() loop on the NALs... Why?
> Also, it seems that you are not putting the first NAL in the extradata?

Sorry, I do not understand "opencoding", can you please explain me ?

SEI nal is not needed in extradata.

> Again, I apologize if I am misunderstanding something, but my impression
> is that your patch does at codec level something that should be done at
> format level... I see "avcC" cited in your libavcodec/x264.c patch, but
> I think it really is a MOV thing? Or am I wrong?
> 

I understand, maybe It should be done in movenc.c. I don't know, maybe
raw H264 streams format could help us on that.

I guess that this AVC thing is actually a MOV/MP4 thing.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312