[MPlayer-dev-eng] [PATCH] - fix for audio and video in dvr-ms asf

[Reimar Döffinger]

Hi,
On Thu, Apr 27, 2006 at 01:20:22PM -0500, John Donaghy wrote:
> > Success! :)
> 
> Thanks for testing it. Hopefully this version will be acceptable.

I can't comment much on the correctness, but it looks pretty clean.
A few small nits:

> +static int find_backwards_asf_guid(char *buf, const char *guid, int cur_pos)
> +{
> +  int i;
> +  for (i=cur_pos; i>0; i--) {
> +    if (memcmp(&buf[i], guid, 16) == 0)
> +      return i + 16 + 8; // point after guid + length
> +  }
> +  return -1;
> +}
> +

I wonder if either a buf_len parameter or starting from cur_pos -
16 might be a good idea, but I guess you know better.
But this reminds reminds me: please add doxygen comments as described in
DOCS/tech/code-documentation.txt, so at least assumptions like "cur_pos
point at most to a position 16 bytes before the end of the buffer" are
documentation, because otherwise I'm sure somebody will sooner or later
miss that little detail.

> @@ -156,7 +204,10 @@
>    int best_video = -1;
>    int best_audio = -1;
>    uint64_t data_len;
> -
> +  ASF_stream_header_t *streamh;
> +  uint8_t *buffer;
> +  int audio_pos=0;
> +  
>    if(hdr_len < 0) {

cosmetics (and a bad one at that *g*), you added trailing whitespace to
the empty line.

> -  {
> -    ASF_stream_header_t *streamh = (ASF_stream_header_t *)&hdr[pos];
> -    uint8_t *buffer;
> +  {    
> +    streamh = (ASF_stream_header_t *)&hdr[pos];

cosmetics as well, what did you change in that { line?

> @@ -30,6 +35,28 @@
>  			 ((unsigned char*)(p))[1]<<8)
>  #endif
>  
> +#if defined(ARCH_X86) || defined(ARCH_X86_64)
> +#    define unaligned32(a) (*(const uint32_t*)(a))
> +#else
> +#    ifdef __GNUC__
> +static inline uint32_t unaligned32(const void *v) {
> +    struct Unaligned {
> +        uint32_t i;
> +    } __attribute__((packed));
> +
> +    return ((const struct Unaligned *) v)->i;
> +}
> +#    elif defined(__DECC)
> +static inline uint32_t unaligned32(const void *v) {
> +    return *(const __unaligned uint32_t *) v;
> +}
> +#    else
> +static inline uint32_t unaligned32(const void *v) {
> +    return *(const uint32_t *) v;
> +}
> +#    endif
> +#endif //!ARCH_X86
> +

Ugh. Scary. Why can't you use the LOAD_LE32 macros? Or alternatively
make similar LOAD_BE32 macros? This at least looks like both a
portability and maintainability macros - at best acceptable in highly
performance-critical code (if it makes a performance difference at all).

> @@ -105,16 +225,10 @@
>          ds_add_packet(ds,ds->asf_packet);
>          ds->asf_packet=NULL;
>        } else {
> -        // append data to it!
> -        demux_packet_t* dp=ds->asf_packet;
> -        if(dp->len!=offs && offs!=-1) mp_msg(MSGT_DEMUX,MSGL_V,"warning! fragment.len=%d BUT next fragment offset=%d  \n",dp->len,offs);
> -        dp->buffer=realloc(dp->buffer,dp->len+len+FF_INPUT_BUFFER_PADDING_SIZE);
> -        memcpy(dp->buffer+dp->len,data,len);
> -        memset(dp->buffer+dp->len+len, 0, FF_INPUT_BUFFER_PADDING_SIZE);
> -        mp_dbg(MSGT_DEMUX,MSGL_DBG4,"data appended! %d+%d\n",dp->len,len);
> -        dp->len+=len;
> -        // we are ready now.
> -        return 1;
> +         // append data to it!
> +         demux_asf_append_to_packet(dp,data,len,offs);
> +         // we are ready now.
> +         return 1;

Some cosmetics here as well.

Greetings,
Reimar Döffinger