[FFmpeg-devel] add MJPEG support into RTP output

[shawn shawn]

Hi,

2010/4/8 Luca Abeni <lucabe72 at email.it>:
> Hi,
>
> Nash Tsai wrote:
>>
>> On Thu, Apr 8, 2010 at 4:18 PM, Martin Storsj? <martin at martin.st> wrote:
>>>
>>> On Thu, 8 Apr 2010, Luca Abeni wrote:
>>>
>>>> Martin Storsj? wrote:
>>>> [...]
>>>>>
>>>>> All current packetizers buffer the data they're going to send within
>>>>> RTPMuxContext->buf, but splitting ff_rtp_send_data this way does allow
>>>>> us to
>>>>> avoid one layer of buffering, if each individual packetizer wants to.
>>>>> You
>>>>> need to hear from Luca A if he's ok with this approach.
>>>>
>>>> Well, I am not sure about this. I am currently undecided, but it other
>>>> people like the idea of splitting ff_rtp_send_data(), I am ok with it.
>>>> Let's just be sure that there are enough people in favour of this
>>>> solution.
>>>
>>> I'm a bit undecided, too. I like the current one for simplicity, but it
>>> does indeed require one extra copy of data.
>>
>> What I can see here is that it's trying to reuse the implementations
>> of writing RTP header and update seq number on what is already inside
>> ff_rtp_send_data(), and have better performance without making a copy
>> of data, isn't this be better?
>
> As it's done in the current patch, no:
> - If there is a performance problem, then it must be fixed for all the
> ?packetisers. Introducing a new one that does things in a different way
> ?respect to the others is not a good idea.
> - If there is not a performance problem (I do not know, I did not see
> ?any number), then optimising this part of the code is not useful.
>
> So, I think that first we need to decide if a new approach (splitting
> ff_rtp_send_data()) is needed; if it is, then we need a small patch
> performing the split, followed by a patch converting the current
> packetisers. Then, the new one (implementing support for the MJPEG
> payload type) can be committed.
>
>
>>>> In any case, what I do not want to see is a non-uniformity between the
>>>> various packetisers. If we decide to split ff_rtp_send_data(), then all
>>>> the packetisers must be converted. Having some packetisers implemented
>>>> in
>>>> the old way and some in the new one is (IMHO) not acceptable.
>>>
>>> Yes, uniformity is important.
>>
>> I tend to agree this, but with sacrifice of performance it's rather
>> hard for me to fully agree on it.
>
> Noone wants to sacrifice performance :)
> But if there are performance problems for the MJPEG packetiser, then
> the same problems are there for all the other packetisers. Fixing the
> performance issues only in one case is not a good idea. And people
> implementing support for new payload types in the future might use the
> "bad" (badly performing) code as a template.
>
> So, if we want to avoid an extra memcpy() let's avoid it for all the
> payload types, not only for a random one...
>
>
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?Luca
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>

   According to all said, i have change it,
 1. use ff_rtp_send_data, not split it
 2. change a=rtpmap:%d MJPEG/30000\r\n to a=rtpmap:%d JPEG/90000\r\n
 3. tidy up some code and add some comment about rfc number
 4. add a new file rtpenc_mjpeg.c
 5.  "tspec", "type", and "q" fields always set to 0, because rfc
defined a range, maybe we can set some input parameters, but now i
have not do it.
 I have test VLC media player, add this header it not work, if don't
add it can work, However, according to rfc documents 2435,
i think we should add it.

Thank you for everyone's reply

rensheng_li
-------------- next part --------------
A non-text attachment was scrubbed...
Name: submission.diff
Type: application/octet-stream
Size: 5312 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100408/3ae9c300/attachment.obj>