[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