[FFmpeg-devel] [PATCH v3] avformat/dashdec: add dash demuxer base version
Clément Bœsch
u at pkh.me
Mon Mar 20 23:01:29 EET 2017
On Mon, Mar 20, 2017 at 10:29:48PM +0800, Steven Liu wrote:
> v2 fixed:
> 1. from autodetect to disabled
> 2. from camelCase code style to ffmpeg code style
> 3. from RepType to AVMediaType
> 4. fix variable typo
> 5. change time value from uint32_t to uint64_t
> 6. removed be used once API
> 7. change 'time(NULL)`, except it is not 2038-safe.' to av_gettime and av_timegm
> 8. merge complex free operation to free_fragment
> 9. use API from snprintf to av_asprintf
>
> v3 fixed:
> 1. fix typo from --enabled-xml2 to --enable-xml2
>
> Signed-off-by: Steven Liu <lq at chinaffmpeg.org>
> ---
> configure | 4 +
> libavformat/Makefile | 1 +
> libavformat/allformats.c | 2 +-
> libavformat/dashdec.c | 1844 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1850 insertions(+), 1 deletion(-)
> create mode 100644 libavformat/dashdec.c
>
> diff --git a/configure b/configure
> index 4eb116b..239a3a9 100755
> --- a/configure
> +++ b/configure
> @@ -273,6 +273,7 @@ External library support:
> --enable-libxcb-shape enable X11 grabbing shape rendering [autodetect]
> --enable-libxvid enable Xvid encoding via xvidcore,
> native MPEG-4/Xvid encoder exists [no]
> + --enable-xml2 enable XML parsing using the C library libxml2 [no]
please use --enable-libxml2 to be consistent with the other libs.
[...]
> +#include "libavutil/avstring.h"
> +#include "libavutil/avassert.h"
> +#include "libavutil/intreadwrite.h"
> +#include "libavutil/mathematics.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/dict.h"
> +#include "libavutil/time.h"
> +#include "libavutil/parseutils.h"
> +#include "avformat.h"
> +#include "internal.h"
> +#include "avio_internal.h"
> +#include "url.h"
> +#include "id3v2.h"
> +
not saying it's wrong, but do you really need all these includes?
> +#define INITIAL_BUFFER_SIZE 32768
> +
> +#include <libxml/parser.h>
> +#include <libxml/tree.h>
> +#include <stdint.h>
> +#include <time.h>
> +#include <unistd.h>
system includes belongs on top
[...]
> +enum TEMP_URL_TYPE {
no capitalization of the enum name
> + TMP_URL_TYPE_UNSPECIFIED,
> + TMP_URL_TYPE_NUMBER,
> + TMP_URL_TYPE_TIME
please add a trailing comma for the last entry
> +};
> +
> +/*
> + * Each playlist has its own demuxer. If it currently is active,
I'm not a native speaker but I think "is" belongs before "currently"
> + * it has an open AVIOContext too, and potentially an AVPacket
opened
> + * containing the next packet from this stream.
> + */
[...]
> +typedef struct DASHContext {
> + AVClass *class;
this should probably be const
[...]
> +static uint64_t GetUTCDateTimeInSec(const char *datetime)
> +{
> + struct tm timeinfo;
> + int year = 0;
> + int month = 0;
> + int day = 0;
> + int hour = 0;
> + int minute = 0;
> + float second = 0.0;
> +
> + /* ISO-8601 date parser */
> + if (!datetime)
> + return 0;
> +
> + sscanf(datetime, "%d-%d-%dT%d:%d:%fZ", &year, &month, &day, &hour, &minute, &second);
you should probably warn if it doesn't recognized enough arguments
> + timeinfo.tm_year = year - 1900;
> + timeinfo.tm_mon = month - 1;
> + timeinfo.tm_mday = day;
> + timeinfo.tm_hour = hour;
> + timeinfo.tm_min = minute;
> + timeinfo.tm_sec = (int)second;
> +
> + return av_timegm(&timeinfo);
> +}
> +
> +static uint32_t GetDurationInSec(const char *duration)
> +{
> + /* ISO-8601 duration parser */
> + uint32_t days = 0;
> + uint32_t hours = 0;
> + uint32_t mins = 0;
> + uint32_t secs = 0;
> + uint32_t size = 0;
> + float value = 0;
> + uint8_t type = 0;
> + const char *ptr = duration;
> +
> + while (*ptr) {
> + if (*ptr == 'P' || *ptr == 'T') {
> + ptr++;
> + continue;
> + }
> +
> + if (sscanf(ptr, "%f%c%n", &value, &type, &size) != 2) {
> + return 0; /* parser error */
> + }
> + switch (type) {
> + case 'D':
> + days = (uint32_t)value;
> + break;
> + case 'H':
> + hours = (uint32_t)value;
> + break;
> + case 'M':
> + mins = (uint32_t)value;
> + break;
> + case 'S':
> + secs = (uint32_t)value;
> + break;
> + default:
> + // handle invalid type
> + break;
> + }
> + ptr += size;
> + }
> + return ((days * 24 + hours) * 60 + mins) * 60 + secs;
> +}
Why not strftime?
> +
> +static void free_fragment(struct fragment **seg)
> +{
> + if (!seg || !(*seg)) {
seg will never be NULL unless you did something very wrong in the caller
which your compiler will warn about
> + return;
> + }
> + av_freep(&(*seg)->url);
> + av_freep(seg);
> +}
> +
> +static void free_fragment_list(struct representation *pls)
> +{
> + int i = 0;
that initialization is pointless
> +
> + for (i = 0; i < pls->n_fragments; i++) {
> + free_fragment(&pls->fragments[i]);
> + }
> + av_freep(&pls->fragments);
> + pls->n_fragments = 0;
> +}
> +
[...]
> +static void update_options(char **dest, const char *name, void *src)
> +{
> + av_freep(dest);
> + av_opt_get(src, name, AV_OPT_SEARCH_CHILDREN, (uint8_t**)dest);
> + if (*dest && !strlen(*dest))
!strlen(x) could be replaced with !x[0]. Not sure why you would only free
if the string is empty though.
> + av_freep(dest);
> +}
> +
[...]
> + if (type == AVMEDIA_TYPE_VIDEO) {
> + video_rep_idx++;
> + }
> + if (type == AVMEDIA_TYPE_AUDIO) {
> + audio_rep_idx++;
> + }
nit:
video_rep_idx += type == AVMEDIA_TYPE_VIDEO;
audio_rep_idx += type == AVMEDIA_TYPE_AUDIO;
> +
> +faild:
strange label name
> + if (rep_id_val)
> + xmlFree(rep_id_val);
> + if (rep_bandwidth_val)
> + xmlFree(rep_bandwidth_val);
> +
> + return ret;
> +}
> +
[...]
Damn that patch is huge :)
I'll leave the rest of the review to others. Thanks for the patch.
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170320/be4f7975/attachment.sig>
More information about the ffmpeg-devel
mailing list