[FFmpeg-devel] [PATCH] RTP H.264 / AVC support

[Howard Chu]

Luca Abeni wrote:
> On 05/30/2010 08:41 PM, Howard Chu wrote:
>> Luca Abeni wrote:
>>> On Fri, 2010-05-28 at 21:12 -0700, Howard Chu wrote:
>>>> In particular, h264_mp4toannexb_bsf is not relevant. RTP sends raw
>>>> NAL units,
>>>> not H.264 Annex B byte streams.
>>>
>>> Sigh... I give up :(
>>> In a previous email I already explained that the RTP muxer gets full
>>> frames as input, and needs to split them in NALs.
>>
>> Yes, understood. I just don't believe that the right solution for AVC
>> format frames is to copy them into a newly malloc'd buffer so that a
>> startcode can be inserted which is just going to be stripped off again.
>
> This is not what you said in the email I was replying to (read about 10
> lnes above :).
> Anyway, as I wrote I give up on using bitstream filters.
>
>
> [...]
>>> This function seems to make everything more complex. What you
>>> need to do is to factorise the code you copied from h264.c, not to
>>> introduce this new function.
>>
>> I've tried to reduce the duplication as much as possible. But if you
>> refer back to the previous patch I posted
>>
>> http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100518/b5cd4c64/attachment.txt
>>
>>
>> Only 3 lines of code in rtpenc.c were copied literally from h264.c.
>
> Regarding the previous patch you are mentioning: yes, maybe only 3 lines
> are literally copied. But then look at the modifications you introduced
> to ff_rtp_send_h264(), and compare them with libavcodec/h264.c:decode_nal_units()
> (around line 2768 and following...).

As I said before, it would be extremely messy to factorize that code out of 
decode_nal_units(). Have you tried?

> Moreover, the code introduced in ff_rtp_send_h264() and in sdp.c:extradata2psets()
> did the same thing (split an "AVC format" frame in NALs) using slightly
> different code (compare "len = (len<<  8) | *r++;" with "nalsize = AV_RB16(r);").

The code differs for good reason. You'll note in the comments in h264.c that 
the sps and pps in the extradata always have nal_length_size == 2, thus the 
code in extradata2psets() only handles a length of 2. The code in 
ff_rtp_send_h264() has to work with any nal_length_size.

> Regarding the new patch: as I said, I do not think this
> ff_avc_parse_nal_units_inplace() is the correct thing to simplify the code.

It certainly simplified ff_rtp_send_h264(), and it could also simplify all of 
the other muxers that handle H264. grep tells me: flvenc.c, matroskaenc.c, 
movenc.c, mpegts.c, nutenc.c.

> (moreover, I am still convinced that this function does the same things
> done by the h264.c code I mentioned above).

Yes, it duplicates some of that functionality. But decode_nal_units() has a 
lot of dependencies on the sequence it operates in, making it messy to 
encapsulate separately. Since this function is in the critical path for 
decoding, and any patch here would be complex and time-consuming to write and 
to review, I've decided to leave it alone. If you want to try factoring out 
the relevant code, go ahead, I'll be happy to make use of it.

> Cannot you have something like a ff_avc_find_startcode_avc() (bad name,
> I know...), which does the same thing done by ff_avc_find_startcode(), but
> for the "AVC" bitstream format?

That only solves part of the problem, the caller still has to check first to 
see whether to use the AVC function or not. If you really want to reduce code 
duplication then all of this belongs in one function so that callers don't 
need to test which one to call.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/