[FFmpeg-devel] [PATCH 1/7] avformat/rtpdec_ac3: add AC3 RTP depacketization
Thomas Volkert
silvo at gmx.net
Thu Feb 12 22:07:39 CET 2015
On 02/08/2015 10:22 PM, Gilles Chanteperdrix wrote:
> ---
> libavformat/Makefile | 1 +
> libavformat/rtpdec.c | 1 +
> libavformat/rtpdec_ac3.c | 166 +++++++++++++++++++++++++++++++++++++++++++
> libavformat/rtpdec_formats.h | 1 +
> 4 files changed, 169 insertions(+)
> create mode 100644 libavformat/rtpdec_ac3.c
>
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index 6bf0761..5acb292 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -30,6 +30,7 @@ OBJS-$(CONFIG_RIFFENC) += riffenc.o
> OBJS-$(CONFIG_RTPDEC) += rdt.o \
> rtp.o \
> rtpdec.o \
> + rtpdec_ac3.o \
> rtpdec_amr.o \
> rtpdec_asf.o \
> rtpdec_g726.o \
> diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
> index 33205aa..f5557d8 100644
> --- a/libavformat/rtpdec.c
> +++ b/libavformat/rtpdec.c
> @@ -66,6 +66,7 @@ void ff_register_dynamic_payload_handler(RTPDynamicProtocolHandler *handler)
>
> void ff_register_rtp_dynamic_payload_handlers(void)
> {
> + ff_register_dynamic_payload_handler(&ff_ac3_dynamic_handler);
> ff_register_dynamic_payload_handler(&ff_amr_nb_dynamic_handler);
> ff_register_dynamic_payload_handler(&ff_amr_wb_dynamic_handler);
> ff_register_dynamic_payload_handler(&ff_g726_16_dynamic_handler);
> diff --git a/libavformat/rtpdec_ac3.c b/libavformat/rtpdec_ac3.c
> new file mode 100644
> index 0000000..2a47b75
> --- /dev/null
> +++ b/libavformat/rtpdec_ac3.c
> @@ -0,0 +1,166 @@
> +/*
> + * RTP parser for AC3 payload format
I would suggest to mention the RFC similar to other parsers, e.g.:
"RTP parser for AC3 payload format (RFC 4184)"
> + * Copyright (c) 2015 Gilles Chanteperdrix <gch at xenomai.org>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "avformat.h"
> +#include "rtpdec_formats.h"
> +#include "libavcodec/get_bits.h"
> +
Okay.
> +/*
> + * From:
> + * https://tools.ietf.org/html/draft-ietf-avt-rtp-ac3-07
> + */
Could you remove this and mention the RFC earlier as suggested above?
(I would rather refer to rfc4184 from 2005 instead of the last draft
version.)
> +
> +struct PayloadContext {
> + unsigned nr_frames;
> + unsigned last_frame;
> + uint32_t timestamp;
> + AVIOContext *fragment;
> +};
> +
Okay.
> +static av_cold int
> +ac3_init(AVFormatContext *s, int st_index, PayloadContext *data)
I would not break the line right after the "int".
> +{
> + if (st_index < 0)
> + return 0;
> + s->streams[st_index]->need_parsing = AVSTREAM_PARSE_FULL;
> + return 0;
> +}
> +
> +static PayloadContext *ac3_new_context(void)
> +{
> + return av_mallocz(sizeof(PayloadContext));
> +}
> +
Okay.
> +static inline void free_fragment_if_needed(PayloadContext *data)
> +{
> + if (data->fragment) {
> + uint8_t *p;
> + avio_close_dyn_buf(data->fragment, &p);
> + av_free(p);
> + data->fragment = NULL;
> + }
> +}
This sequence is used for several RTP parsers now.
Maybe we should create a general function for these lines (separate
patch)...
> +
> +static void ac3_free_context(PayloadContext *data)
> +{
> + free_fragment_if_needed(data);
> + av_free(data);
> +}
> +
> +static int ac3_handle_packet(AVFormatContext *ctx, PayloadContext *data,
> + AVStream *st, AVPacket *pkt, uint32_t *timestamp,
> + const uint8_t *buf, int len, uint16_t seq,
> + int flags)
Instead of "data" you could use "pl_ctx". But this is more or less my
personal opinion.
> +{
> + unsigned frame_type;
> + unsigned nr_frames;
> + int err;
> +
> + if (len < 2) {
How about a definition like "#define RTP_AC3_PAYLOAD_HEADER_SIZE 2" at
the beginning of the file and use this term here again?
Is it possible that a packet without any AC3 payload is received which
should be forwarded to the decoder anyway?
Otherwise, I would rather check for "len < RTP_AC3_PAYLOAD_HEADER_SIZE + 1".
> + av_log(ctx, AV_LOG_ERROR, "Invalid %d bytes packet\n", len);
> + return AVERROR_INVALIDDATA;
> + }
> +
> + frame_type = buf[0] & 0x3;
> + nr_frames = buf[1];
Okay.
> + buf += 2;
> + len -= 2;
RTP_AC3_PAYLOAD_HEADER_SIZE again?
> +
> + switch (frame_type) {
> + case 0: /* One or more complete frames */
> + if (nr_frames > 1) {
> + av_log(ctx, AV_LOG_ERROR,
> + "Unimplemented multiple AC3 frames per packet\n");
You could use avpriv_report_missing_feature() here.
> + return AVERROR_PATCHWELCOME;
> + }
> + if (nr_frames == 0) {
> + av_log(ctx, AV_LOG_ERROR, "Invalid AC3 packet data\n");
> + return AVERROR_INVALIDDATA;
> + }
> + if (av_new_packet(pkt, len)) {
> + av_log(ctx, AV_LOG_ERROR, "Out of memory.\n");
> + return AVERROR(ENOMEM);
> + }
> +
> + pkt->stream_index = st->index;
> + memcpy(pkt->data, buf, len);
> + return 0;
> +
> + case 1:
> + case 2: /* First fragment */
> + free_fragment_if_needed(data);
> +
> + data->last_frame = 1;
> + data->nr_frames = nr_frames;
> + err = avio_open_dyn_buf(&data->fragment);
> + if (err < 0)
> + return err;
> +
> + avio_write(data->fragment, buf, len);
> + data->timestamp = *timestamp;
> + return AVERROR(EAGAIN);
> +
> + case 3: /* Fragment other than first */
> + if (!data->fragment) {
> + av_log(ctx, AV_LOG_WARNING,
> + "Received packet without a start fragment; dropping.\n");
> + return AVERROR(EAGAIN);
> + }
> + if (nr_frames != data->nr_frames ||
> + data->timestamp != *timestamp) {
> + free_fragment_if_needed(data);
> + av_log(ctx, AV_LOG_ERROR, "Invalid packet received\n");
> + return AVERROR_INVALIDDATA;
> + }
> +
> + avio_write(data->fragment, buf, len);
> + data->last_frame++;
> + }
> +
> + if (!(flags & RTP_FLAG_MARKER))
> + return AVERROR(EAGAIN);
> +
> + if (data->last_frame != data->nr_frames) {
> + free_fragment_if_needed(data);
> + av_log(ctx, AV_LOG_ERROR, "Missed %d packets\n",
> + data->nr_frames - data->last_frame);
> + return AVERROR_INVALIDDATA;
> + }
> +
> + err = ff_rtp_finalize_packet(pkt, &data->fragment, st->index);
> + if (err < 0) {
> + av_log(ctx, AV_LOG_ERROR,
> + "Error occurred when getting fragment buffer.");
> + return err;
> + }
> +
> + return 0;
> +}
> +
Okay.
> +RTPDynamicProtocolHandler ff_ac3_dynamic_handler = {
> + .enc_name = "ac3",
> + .codec_type = AVMEDIA_TYPE_AUDIO,
> + .codec_id = AV_CODEC_ID_AC3,
> + .init = ac3_init,
> + .alloc = ac3_new_context,
> + .free = ac3_free_context,
> + .parse_packet = ac3_handle_packet,
> +};
Do you see any use for the optional SDP parameter?
If yes, you could add an AC3 specific parser for SDP line and link it as
".parse_sdp_a_line" line here.
> diff --git a/libavformat/rtpdec_formats.h b/libavformat/rtpdec_formats.h
> index 803410e..e5aff59 100644
> --- a/libavformat/rtpdec_formats.h
> +++ b/libavformat/rtpdec_formats.h
> @@ -39,6 +39,7 @@ int ff_h263_handle_packet(AVFormatContext *ctx, PayloadContext *data,
> AVStream *st, AVPacket *pkt, uint32_t *timestamp,
> const uint8_t *buf, int len, uint16_t seq, int flags);
>
> +extern RTPDynamicProtocolHandler ff_ac3_dynamic_handler;
> extern RTPDynamicProtocolHandler ff_amr_nb_dynamic_handler;
> extern RTPDynamicProtocolHandler ff_amr_wb_dynamic_handler;
> extern RTPDynamicProtocolHandler ff_g726_16_dynamic_handler;
please add:
- "Changelog" entry
- bump minor / micro version in version.h
- "MAINTAINERS" entry (if you will also be available as maintainer of
your file)
What do you think about a packetizer as counterpart?
Best regards,
Thomas.
More information about the ffmpeg-devel
mailing list