[FFmpeg-devel] [PATCH] Added an Adobe HTTP Dynamic Streaming (HDS) demuxer
CORY MCCARTHY
cory.mccarthy at shaw.ca
Wed Nov 20 02:19:45 CET 2013
Added Changelog entry
Use the existing amf_get_string() and remove duplicate amf_metadata_read_string()
Use avio_get_str() and remove f4fbox_read_string()
Separated declarations and statements
Use sizeof() instead of hard coded literal numbers
Fixed typo with "libxml" includes
Use av_freep instead of av_free to prevent stale pointers
Pass output buffer length to av_strlcat() instead of input buffer length
Remove unneeded cast
Remove unneeded pix_fmt set
Use avpriv_set_pts_info
I attached a patch that shows these changes.
I'll work on addressing the rest of the issues outlined below.
Thank you,
Cory
----- Original Message -----
From: "Michael Niedermayer" <michaelni at gmx.at>
To: "FFmpeg development discussions and patches" <ffmpeg-devel at ffmpeg.org>
Sent: Monday, November 18, 2013 8:01:13 PM
Subject: Re: [FFmpeg-devel] [PATCH] Added an Adobe HTTP Dynamic Streaming (HDS) demuxer
On Mon, Nov 18, 2013 at 04:14:30PM -0700, CORY MCCARTHY wrote:
> Added support for playback of Adobe HDS streams.
> FFplay can be used as an Adobe HDS player client.
>
> The user provides a link to the F4M Manifest file, which describes a set of HDS streams.
> The HDS demuxer creates an AVProgram for each different media quality level.
> The demuxer will then request, download and parse F4F files for the selected program/quality level.
>
> The F4M Manifest file is an XML-based format.
> The external library libxml2 is used to help parse the manifest file.
> The library is auto-detected.
>
> Increased libavformat minor version number.
> configure | 8
> libavformat/Makefile | 1
> libavformat/allformats.c | 2
> libavformat/amfmetadata.c | 202 ++++++++++++++
> libavformat/amfmetadata.h | 45 +++
> libavformat/f4fbox.c | 323 ++++++++++++++++++++++
> libavformat/f4fbox.h | 101 +++++++
> libavformat/f4mmanifest.c | 265 ++++++++++++++++++
> libavformat/f4mmanifest.h | 64 ++++
> libavformat/flvtag.c | 390 +++++++++++++++++++++++++++
> libavformat/flvtag.h | 39 ++
> libavformat/hdsdec.c | 661 ++++++++++++++++++++++++++++++++++++++++++++++
> libavformat/version.h | 2
> 13 files changed, 2101 insertions(+), 2 deletions(-)
> b11be02e1124518cdf6b4d37ba73d777c4c2c687 hdsdec.patch
> configure | 8 +
> libavformat/Makefile | 1 +
> libavformat/allformats.c | 2 +-
> libavformat/amfmetadata.c | 202 ++++++++++++++
> libavformat/amfmetadata.h | 45 ++++
> libavformat/f4fbox.c | 323 ++++++++++++++++++++++
> libavformat/f4fbox.h | 101 +++++++
> libavformat/f4mmanifest.c | 265 +++++++++++++++++++
> libavformat/f4mmanifest.h | 64 +++++
> libavformat/flvtag.c | 390 +++++++++++++++++++++++++++
> libavformat/flvtag.h | 39 +++
> libavformat/hdsdec.c | 661 ++++++++++++++++++++++++++++++++++++++++++++++
> libavformat/version.h | 2 +-
Changelog entry missing
[...]
> +static int amf_metadata_read_string(AVIOContext *in, char *str, int str_size)
> +{
> + uint16_t size;
> + int ret;
> +
> + size = avio_rb16(in);
> +
> + if(size + 1 > str_size) {
> + av_log(NULL, AV_LOG_ERROR, "amfmetadata String too large, size: %u \n", size);
> + return -1;
> + }
> +
> + if(size > 0 && (ret = avio_read(in, str, size)) < 0) {
> + av_log(NULL, AV_LOG_ERROR, "amfmetadata Failed to read string, ret: %d \n", ret);
> + return ret;
> + }
> +
> + str[size] = '\0';
> + return 0;
> +}
this can possibly be factorized with amf_get_string()
similar factorizations might be possible woth other amf/flv code
[...]
> +static void f4fbox_read_string(AVIOContext *in, char *str, int str_size)
> +{
> + char *p;
> + int i;
> +
> + p = str;
> + for(i = 0; i < str_size; i++) {
> + *p = avio_r8(in);
> + if(!*p)
> + break;
> + p++;
> + }
> +}
avio_get_str()
[...]
> +static int f4fbox_parse_abst(AVIOContext *in, int64_t data_size, void *opague)
> +{
> + F4FBox *parent = (F4FBox*)opague;
> + F4FBootstrapInfoBox *abst = &(parent->abst);
> + uint8_t server_entry_count, quality_entry_count;
> + uint8_t segment_run_table_count, fragment_run_table_count;
> + char url[1024];
> + int i, ret;
> +
> + abst->version = avio_r8(in);
> + abst->flags = avio_rb24(in);
> + abst->bootstrap_info_version = avio_rb32(in);
> +
> + uint8_t byte = avio_r8(in);
mixed declaration and statements, some compilers dislike these
> + abst->profile = (byte >> 6) & 0x03;
> + abst->is_live = (byte >> 5) & 0x01;
> + abst->is_update = (byte >> 4) & 0x01;
> +
> + abst->timescale = avio_rb32(in);
> + abst->current_media_time = avio_rb64(in);
> + abst->smpte_time_code_offset = avio_rb64(in);
> +
> + f4fbox_read_string(in, abst->movie_id, 1024);
> +
> + server_entry_count = avio_r8(in);
> + for(i = 0; i < server_entry_count; i++) {
> + f4fbox_read_string(in, url, 1024);//ignore
sizeof() is safer than hardcoded litteral numbers
> + }
> +
> + quality_entry_count = avio_r8(in);
> + for(i = 0; i < quality_entry_count; i++) {
> + f4fbox_read_string(in, url, 1024);//ignore
> + }
> +
> + f4fbox_read_string(in, abst->drm_data, 1024);
> + f4fbox_read_string(in, abst->metadata, 1024);
> +
> + segment_run_table_count = avio_r8(in);
> + for(i = 0; i < segment_run_table_count; i++) {
> + if((ret = f4fbox_parse_single_box(in, abst)) < 0) {
> + av_log(NULL, AV_LOG_ERROR, "f4fbox Failed to parse asrt box, ret: %d \n", ret);
av_log() should always have some context so the user (or developer)
know where it comes from
[...]
> +int ff_parse_f4f_box(uint8_t *buffer, int buffer_size, F4FBox *box);
> +int ff_free_f4f_box(F4FBox *box);
> diff --git a/libavformat/f4mmanifest.c b/libavformat/f4mmanifest.c
> new file mode 100644
> index 0000000..c22ac3e
> --- /dev/null
> +++ b/libavformat/f4mmanifest.c
> @@ -0,0 +1,265 @@
> +/*
> + * Adobe Media Manifest (F4M) File Parser
> + * Copyright (c) 2013 Cory McCarthy
> + *
> + * 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
> + */
> +
> +/**
> + * @file
> + * @brief Adobe Media Manifest (F4M) File Parser
> + * @author Cory McCarthy
> + * @see http://wwwimages.adobe.com/www.adobe.com/content/dam/Adobe/en/devnet/hds/pdfs/adobe-media-manifest-specification.pdf
> + */
> +
> +#include "f4mmanifest.h"
> +#include "libavutil/avstring.h"
> +#include "libavutil/base64.h"
> +#include "libxml/parser.h"
> +#include "libxml/tree.h"
i assume this is meant to be <libxml/....h>
[...]
> +static int flv_tag_parse_video_body(AVIOContext *in, uint32_t data_size,
> + FLVTagVideoHeader *header, FLVTagVideoBody *body, FLVMediaSample **sample_out)
> +{
> + FLVMediaSample *sample = NULL;
> + uint8_t *p;
> + int i, ret = 0;
> +
> + if(header->frame_type == 0x05) {
> + avio_r8(in);
> + return 1;
> + }
> +
> + if(header->codec_id != 0x07) {
> + av_log(NULL, AV_LOG_ERROR, "flvtag Unhandled video codec id, id: %d \n", header->codec_id);
> + return 0;
> + }
> +
> + if(header->avc_packet_type == 0x00) {
> + body->configuration_version = avio_r8(in);
> + body->avc_profile_indication = avio_r8(in);
> + body->profile_compatibility = avio_r8(in);
> + body->avc_level_indication = avio_r8(in);
> + ret += 4;
> +
> + body->length_size_minus_one = avio_r8(in) & 0x03;
> + ret++;
> +
> + if(body->sps_data_size > 0)
> + av_free(body->sps_data);
> + if(body->pps_data_size > 0)
> + av_free(body->pps_data);
if you use av_freep() instead of av_free() then no stale pointers
remain after deallocation
> +
> + uint8_t nb_sps = avio_r8(in) & 0x1F;
> + ret++;
> +
> + for(i = 0; i < nb_sps; i++) {
> + uint16_t sps_length = avio_rb16(in);
> + ret += 2;
> +
> + body->sps_data = av_realloc(body->sps_data,
> + body->sps_data_size + sps_length + 4);
> + if(!body->sps_data)
> + return AVERROR(ENOMEM);
> +
> + p = body->sps_data + body->sps_data_size;
> +
> + *p++ = 0x00;
> + *p++ = 0x00;
> + *p++ = 0x00;
> + *p++ = 0x01;
> + body->sps_data_size += 4;
> +
> + avio_read(in, p, sps_length);
> + body->sps_data_size += sps_length;
> +
> + ret += sps_length;
> + }
> +
> + uint8_t nb_pps = avio_r8(in);
> + ret++;
> +
> + for(i = 0; i < nb_pps; i++) {
> + uint16_t pps_length = avio_rb16(in);
> + ret += 2;
> +
> + body->pps_data = av_realloc(body->pps_data,
> + body->pps_data_size + pps_length + 4);
> + if(!body->pps_data)
> + return AVERROR(ENOMEM);
> +
> + p = body->pps_data + body->pps_data_size;
> +
> + *p++ = 0x00;
> + *p++ = 0x00;
> + *p++ = 0x00;
> + *p++ = 0x01;
> + body->pps_data_size += 4;
> +
> + avio_read(in, p, pps_length);
> + body->pps_data_size += pps_length;
> +
> + ret += pps_length;
> + }
> + }
> + else if(header->avc_packet_type == 0x01) {
> + sample = av_mallocz(sizeof(FLVMediaSample));
> + if(!sample)
> + return AVERROR(ENOMEM);
> +
> + sample->type = AVMEDIA_TYPE_VIDEO;
> +
> + sample->data_size = body->sps_data_size + body->pps_data_size;
> + sample->data_size += 4 + data_size;
> +
> + sample->data = av_mallocz(sizeof(uint8_t) * sample->data_size);
> + if(!sample->data)
> + return AVERROR(ENOMEM);
> +
> + p = sample->data;
> +
> + memcpy(p, body->sps_data, body->sps_data_size);
> + p += body->sps_data_size;
> +
> + memcpy(p, body->pps_data, body->pps_data_size);
> + p += body->pps_data_size;
> +
> + uint32_t nal_size;
> + while(ret < data_size) {
> + *p++ = 0x00;
> + *p++ = 0x00;
> + *p++ = 0x00;
> + *p++ = 0x01;
> +
> + nal_size = avio_rb32(in);
> + ret += 4;
> +
> + avio_read(in, p, nal_size);
this looks like a exploitable out of array write with the right
crafted input
[...]
> +static void construct_bootstrap_url(const char *base_url, const char *bootstrap_url,
> + const char *suffix, char *url_out)
> +{
> + char *p;
> +
> + p = url_out;
> + p += av_strlcat(p, base_url, strlen(base_url)+1);
> + p += av_strlcat(p, bootstrap_url, strlen(bootstrap_url)+1);
> + p += av_strlcat(p, suffix, strlen(suffix)+1);
this design looks unsafe,
av_strlcat() should be given the length of the output buffer not
the input
> +}
> +
> +static int download_bootstrap(AVFormatContext *s, HDSBootstrapInfo *bootstrap,
> + uint8_t **buffer_out, int *buffer_size_out)
> +{
> + HDSContext *c = s->priv_data;
> + URLContext *puc;
> + char url[MAX_URL_SIZE];
> + uint8_t *buffer;
> + int buffer_size;
> + int ret;
> +
> + memset(url, 0x00, MAX_URL_SIZE);
> + construct_bootstrap_url(c->base_url, bootstrap->url, "?hdcore", url);
> +
> + if((ret = ffurl_open(&puc, url, AVIO_FLAG_READ, &s->interrupt_callback, NULL)) < 0) {
> + av_log(NULL, AV_LOG_ERROR, "hds Failed to start downloading bootstrap, ret: %d \n", ret);
> + return ret;
> + }
> +
> + buffer_size = ffurl_size(puc);
> + buffer = (uint8_t*)av_mallocz(buffer_size+FF_INPUT_BUFFER_PADDING_SIZE);
the cast is unneeded
[...]
> +static int create_bootstrap_info(AVFormatContext *s, F4MBootstrapInfo *f4m_bootstrap_info)
> +{
> + HDSContext *c = s->priv_data;
> + HDSBootstrapInfo *bootstrap_info;
> + uint8_t *buffer;
> + int buffer_size, ret;
> +
> + bootstrap_info = av_mallocz(sizeof(HDSBootstrapInfo));
nitpick: sizeof(*bootstrap_info) could be used
> + if(!bootstrap_info)
> + return AVERROR(ENOMEM);
> +
> + c->bootstrap_info[c->nb_bootstraps++] = bootstrap_info;
> +
> + memcpy(bootstrap_info->id, f4m_bootstrap_info->id, MAX_URL_SIZE);
> + memcpy(bootstrap_info->url, f4m_bootstrap_info->url, MAX_URL_SIZE);
> + memcpy(bootstrap_info->profile, f4m_bootstrap_info->profile, MAX_URL_SIZE);
using sizeof(bootstrap_info->id) and similar would be safer
[...]
> +
> +static int create_streams(AVFormatContext *s, HDSMedia *media, AMFMetadata *metadata)
> +{
> + AVStream *st;
> +
> + st = avformat_new_stream(s, NULL);
> + if(!st)
> + return AVERROR(ENOMEM);
> +
> + media->video_stream = st;
> +
> + st->id = 0;
> + st->time_base.num = 1;
> + st->time_base.den = 1000;
> +
> + st->codec->codec_type = AVMEDIA_TYPE_VIDEO;
> + st->codec->codec_id = metadata->video_codec_id;
> + st->codec->width = metadata->width;
> + st->codec->height = metadata->height;
> + st->codec->pix_fmt = PIX_FMT_YUV420P;
this shouldnt be needed
> + st->codec->bit_rate = metadata->video_data_rate * 1000;
> +
> + st = avformat_new_stream(s, NULL);
> + if(!st)
> + return AVERROR(ENOMEM);
> +
> + media->audio_stream = st;
> +
> + st->id = 0;
> + st->time_base.num = 1;
> + st->time_base.den = 1000;
see avpriv_set_pts_info()
[...]
> +static int hds_probe(AVProbeData *p)
> +{
> + if(p->filename && av_stristr(p->filename, ".f4m"))
> + return AVPROBE_SCORE_MAX;
> + return 0;
> +}
to detect by extensions setting AVInputFormat.extension should be
enough
a filecontent based probe would be nice to have though
[...]
Thanks
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel at ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hdsdec_rev2.patch
Type: text/x-patch
Size: 23658 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131119/b2dacc47/attachment.bin>
More information about the ffmpeg-devel
mailing list