[Ffmpeg-devel] RTP patches & RFC
Michael Niedermayer
michaelni
Fri Oct 27 03:07:32 CEST 2006
Hi
On Thu, Oct 26, 2006 at 06:46:19PM -0500, Ryan Martell wrote:
>
> On Oct 26, 2006, at 5:30 PM, Michael Niedermayer wrote:
>
> Some items in this conversation have been reordered.
>
> >ok, IMHO first before anything from the patch below can be applied,
> >the
> >indention of the current code (which was broken by your last patch)
> >must
> >be fixed in a seperate patch
>
[...]
>
> //#define DEBUG
> @@ -187,15 +187,18 @@
> };
>
> /* statistics functions */
> -RTPDynamicProtocolHandler *RTPFirstDynamicPayloadHandler= NULL;
> +RTPDynamicProtocolHandler *RTPFirstDynamicPayloadHandler = NULL;
these change is unneccesary, only the _wrong_ indention should be corrected
for example the one below
[...]
> } else {
> - if (s->read_buf_index >= s->read_buf_size)
and
[...]
> } else {
> - av_new_packet(pkt, len);
> - memcpy(pkt->data, buf, len);
> + av_new_packet(pkt, len);
> + memcpy(pkt->data, buf, len);
> }
note, the goal of all that is to keep svn useable
if indention changes and functional changes are mixed svn-log and patches
become unreadable and unreviewable
if we reindent every second line of code then svn annotate and svn diff will
become much harder to use, also other patches which change the same file
would likely no longer apply
[...]
> /* rtcp sender statistics receive */
> - int64_t last_rtcp_ntp_time; // TODO: move into statistics
> - int64_t first_rtcp_ntp_time; // TODO: move into statistics
> - uint32_t last_rtcp_timestamp; // TODO: move into statistics
> + int64_t last_rtcp_ntp_time; // TODO: move into statistics
> + int64_t first_rtcp_ntp_time; // TODO: move into statistics
> + uint32_t last_rtcp_timestamp; // TODO: move into statistics
the nicely aligned comments get missaligned by the automatic reindent
[...]
> -static attrname_map_t attr_names[]=
> -{
> - {"SizeLength", ATTR_NAME_TYPE_INT, offsetof(rtp_payload_data_t, sizelength)},
> - {"IndexLength", ATTR_NAME_TYPE_INT, offsetof(rtp_payload_data_t, indexlength)},
> - {"IndexDeltaLength", ATTR_NAME_TYPE_INT, offsetof(rtp_payload_data_t, indexdeltalength)},
> - {"profile-level-id", ATTR_NAME_TYPE_INT, offsetof(rtp_payload_data_t, profile_level_id)},
> - {"StreamType", ATTR_NAME_TYPE_INT, offsetof(rtp_payload_data_t, streamtype)},
> - {"mode", ATTR_NAME_TYPE_STR, offsetof(rtp_payload_data_t, mode)},
> +static attrname_map_t attr_names[] = {
> + {"SizeLength", ATTR_NAME_TYPE_INT,
> + offsetof(rtp_payload_data_t, sizelength)},
> + {"IndexLength", ATTR_NAME_TYPE_INT,
> + offsetof(rtp_payload_data_t, indexlength)},
> + {"IndexDeltaLength", ATTR_NAME_TYPE_INT,
> + offsetof(rtp_payload_data_t, indexdeltalength)},
> + {"profile-level-id", ATTR_NAME_TYPE_INT,
> + offsetof(rtp_payload_data_t, profile_level_id)},
> + {"StreamType", ATTR_NAME_TYPE_INT,
> + offsetof(rtp_payload_data_t, streamtype)},
> + {"mode", ATTR_NAME_TYPE_STR, offsetof(rtp_payload_data_t, mode)},
> {NULL, -1, -1},
this also becomes unreadable by the reindent
[...]
>
> (This is simply rtp.c and rtsp.c and rtp_internal.h run through
> gnuindent -i4 -kr -nut).
> (Which results in a huge patch)
and was not what i had in mind ...
[...]
> >[...]
> >> void rtp_parse_close(RTPDemuxContext *s)
> >>Index: libavformat/rtp_internal.h
> >>===================================================================
> >>--- libavformat/rtp_internal.h (revision 6798)
> >>+++ libavformat/rtp_internal.h (working copy)
> >>@@ -23,7 +23,7 @@
> >>
> >> typedef int (*DynamicPayloadPacketHandlerProc) (struct
> >>RTPDemuxContext * s,
> >> AVPacket * pkt,
> >>- uint32_t timestamp,
> >>+ uint32_t *timestamp,
> >> const uint8_t * buf,
> >> int len);
> >>
> >>@@ -82,5 +82,7 @@
> >> };
> >>
> >> extern RTPDynamicProtocolHandler *RTPFirstDynamicPayloadHandler;
> >>+
> >>+int rtsp_next_attr_and_value(const char **p, char *attr, int
> >>attr_size, char *value, int value_size); ///< from rtsp.c, but
> >>used in dynamic packet handlers
> >> #endif /* RTP_INTERNAL_H */
> >>
> >>Index: libavformat/rtsp.c
> >>===================================================================
> >>--- libavformat/rtsp.c (revision 6798)
> >>+++ libavformat/rtsp.c (working copy)
> >>@@ -200,6 +200,8 @@
> >> i = atoi(buf);
> >> if (i > 0)
> >> codec->channels = i;
> >>+ // TODO: there is a bug here; if it is a mono
> >>stream, and less than 22000Hz, faad upconverts to stereo and twice
> >>the
> >>+ // frequency. No problem, but the sample
> >>rate is being set here by the sdp line. Upcoming patch
> >>forthcoming. (rdm)
> >> }
> >> av_log(codec, AV_LOG_DEBUG, " audio samplerate
> >>set to : %i\n", codec->sample_rate);
> >> av_log(codec, AV_LOG_DEBUG, " audio channels set
> >>to : %i\n", codec->channels);
> >>@@ -287,6 +289,22 @@
> >> {NULL, -1, -1},
> >> };
> >>
> >>+int rtsp_next_attr_and_value(const char **p, char *attr, int
> >>attr_size, char *value, int value_size)
> >>+{
> >>+ skip_spaces(p);
> >>+ if(**p)
> >>+ {
> >>+ get_word_sep(attr, attr_size, "=", p);
> >>+ if (**p == '=')
> >>+ (*p)++;
> >>+ get_word_sep(value, value_size, ";", p);
> >>+ if (**p == ';')
> >>+ (*p)++;
> >>+ return 1;
> >>+ }
> >>+ return 0;
> >>+}
> >>+
> >> /* parse a SDP line and save stream attributes */
> >> static void sdp_parse_fmtp(AVStream *st, const char *p)
> >> {
> >>@@ -298,7 +316,8 @@
> >> AVCodecContext *codec = st->codec;
> >> rtp_payload_data_t *rtp_payload_data = &rtsp_st-
> >>>rtp_payload_data;
> >>
> >>- /* loop on each attribute */
> >>+ // TODO (Replace with rtsp_next_attr_and_value)
> >>+ /* loop on each attribute */
> >> for(;;) {
> >> skip_spaces(&p);
> >> if (*p == '\0')
> >>@@ -471,6 +490,19 @@
> >> }
> >> }
> >> }
> >>+ } else if(strstart(p, "framesize:", &p)) {
> >>+ // let dynamic protocol handlers have a stab at the
> >>line.
> >>+ get_word(buf1, sizeof(buf1), &p);
> >>+ payload_type = atoi(buf1);
> >>+ for(i = 0; i < s->nb_streams;i++) {
> >>+ st = s->streams[i];
> >>+ rtsp_st = st->priv_data;
> >>+ if (rtsp_st->sdp_payload_type == payload_type) {
> >>+ if(rtsp_st->dynamic_handler && rtsp_st-
> >>>dynamic_handler->parse_sdp_a_line) {
> >>+ rtsp_st->dynamic_handler->parse_sdp_a_line
> >>(st, rtsp_st->dynamic_protocol_context, buf);
> >>+ }
> >>+ }
> >>+ }
> >> }
> >> break;
> >> }
> >
> >many of the things above look quite generic and not h.264 specific,
> >they
> >could be split into seperate patch(es)
>
> Okay, I'm missing something, because the way i am doing this now is a
> pain in the ass.
yes i feared that, and its not my intent ...
>
> I have my working copy, then I have the latest version in a separate
> directory. I then copy the files from my working copy to the current
> version, and run a file merging tool on them. I then look at every
> line, and take out a bunch of stuff to make the patches smaller.
> Then I have to fix a bunch of stuff that won't compile, because I
> took something out that I needed to leave in. As a result, every
> time I submit a patch, it's taking quite awhile.
>
> I understand the need for small patches, to keep a good idea of where
> the bugs are introduced, but what's the simple way to do this that I
> am missing? I am trying to take small steps, but also trying to take
> big enough steps to get this in into the code base so I can go onto
> to fixing the bugs.
>
> After I submit the patch, I can't work on the next patch until it is
> accepted. (I use svn diff for my patches). I just tried to move the
> AAC stuff out after my indent patch, but then I have to save the
> changes as a different filename, and run diff against that, which
> doesn't make sense, since there is no revision number or anything for
> the patch to apply to.
>
> How can I do this so that I could submit a sequence of patches? (For
> this email, I was going to submit an indent patch, a aac audio
> movement patch, a non-specific rtp/rtp.h change patch, then the h264
> patch)
well that can be done, or at least you can try ...
there are many ways, one is just to have a directory for each "state"
so theres the unchanged-svn-head/myfile.c, ...
and indent-fixup/myfile.c, ....
and indent-fixup-and-aac-move/myfile.c, ...
and then you just use diff between the directories (without any svn involvment)
another solution is to install svn (server) locally and commit to that
that way you can use svn diff
yet another is to hope that the patches wont conflict
so you build your first patch, and then reverse it locally
(svn revert or patch -R) and then build your second as if the first
didnt exist (this of course can fail when we attepmt to apply the patch
if the patches change the same lines of code ...)
and then there are things like git which some people use locally to
simplify their work with patches, sadly i cant say much about that as
ive never really used it, though at least one developer who has used
it successfully submitted large numbers of patches within short time ...
the most ideal case is to submit changes early before they become very
big, yes i know that doesnt help you now ...
[...]
> >>
> >>/**
> >> RTP/H264 specific private data.
> >>*/
> >>typedef struct h264_rtp_extra_data {
> >> unsigned long cookie; ///< sanity check, to make sure we
> >>get the pointer we're expecting.
> >>
> >> struct packet_queue network_packets; ///< network
> >>packets are in this list...
> >> struct packet_queue frame_packets; ///< linked list of all
> >>the packets with the same timestamp.
> >> struct packet_queue out_of_band_packets; ///< pps and
> >>sps... (trasmitted via sdp)
> >> struct packet_queue partial_packets; ///< fragmentation
> >>unit packets.
> >> struct packet_queue packet_pool; ///< preallocated packet
> >>pool; we get them from here first if we need them...
> >
> >one thing ive been curious about is why does h.264 need this mess
> >while the
> >other codecs dont? what is the problem with just removing the extra
> >headers
> >adding the 001 startcode prefixes and then passing the packets
> >through the
> >AVParser? are the packets out of order in some way or not? maybe my
> >question
> >is stupid but i plain dont understand why this complex buffering
> >system is
> >needed for h.264 ...
>
> They aren't out of order with this packetization mode, but if the
> other mode was implemented, it has Decoding Order Numbers in it,
> which would require out of order reordering.
hmm, is this other mode used by anyone? is it mandatory in the sense
that if the decoder doesnt support it its out of luck instead of the
server just switching to the normal mode?
>
> The other part of the issue is that fragmentation packets should be
> reassembled before handing them to the AVParser, so that if there is
> a sequence issue or a missing packet, the entire packet can be
> dropped without going to the codec to corrupt the stream. There is
> no way (IMHO) of doing this, if I just passed the packets up the
> chain. I know the parser is resilient, but basically a packet could
> be broken anywhere. it seems like a lot of strain to put on the
> decoder's error detection/correction, when at my level I KNOW if
> parts were dropped.
well, but the decoder should be able to decode the part of a NAL unit
until the missing part, so droping the whole just isnt correct
but note, i dont know how well this currently works with h264.c, its just
supposed to work and does work with the mpeg/h263 decoders ...
and about receiving packets out of order due to network delay/routing
(=sequence issues?) this is a generic problem and should be solved
(or ignored) generically and not just in rtp-h264.c
[...]
> >base64_decode() should be in a seperate patch
>
> Okay, but where should it be? It's currently only used by the h264
> stuff, so I have it in my h264 code. I did see that there was a
base64.c base64.h in libavformat, we can always move or rename it later
> base64 encode in the source somewhere, but it's static.
that could also be moved into base64.c/h ...
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is
More information about the ffmpeg-devel
mailing list