[FFmpeg-devel] [PATCH] Patches to fix issue453 in libdiracschroedinger

[Michael Niedermayer]

On Thu, May 22, 2008 at 12:26:16PM +1000, Anuradha Suraparaju wrote:
> Hi,
> 
> I've included 2 patches to fix the bug reported in issue453. More
> details of the bug can be found in
> 
> http://roundup.mplayerhq.hu/roundup/ffmpeg/issue453
> 
> The same issue applies to libdirac too. So I included a patch for
> libdirac as well.
> 
> Non-frame data is either prepended (sequence headers) or appended (end
> of sequence info to the last frame) to frame data to ensure that the
> codec outputs frame data in every packet and the pts is monotonous. So a
> packet output by the encoder will contain one encoded frame and header
> or sequence end info when applicable. 
> 
> 1. issue453_fix_pts_bug_common_libdirac_libschroedinger.diff
> 
>    Contains patched files common to libdirac and libschroedinger.
> 
>    libavcodec/libdirac_libschro.c{.h}
> 
>    libavcodec/dirac_parser.c - Since a packet can now contain more than 
>    one Dirac parse unit, a complete packet will still need to be parsed
>    to extract a single Dirac parse unit.
> 
>    libavformat/riff.c   - Add a fourcc code for CODEC_ID_DIRAC to enable
>    wrapping and playback of Dirac in AVI.
> 
> 2. issue453_fix_pts_bug_libschroedinger.diff
> 
>    Contains patch to libschroedingerenc.c to fix the non-monotone pts 
>    bug
> 
> 3. fix_pts_bug_libdirac.diff
> 
>    Contains patch to libdiracenc.c to ensure that pts is monotnous.
[...]
> @@ -104,6 +106,7 @@
>      FfmpegDiracSchroQueueElement *top = queue->p_head;
>  
>      if (top != NULL) {
> +        --queue->size;
>          void *data = top->data;
>          queue->p_head = queue->p_head->next;
>          av_freep (&top);

mixes declaration and statement thus breaks gcc 2.95 support.


> @@ -112,3 +115,8 @@
>  
>      return NULL;
>  }
> +
> +int32_t ff_dirac_schro_queue_size (FfmpegDiracSchroQueue *queue)
> +{
> +    return queue->size;
> +}

Senseless wraper function.


> Index: libavcodec/libdirac_libschro.h
> ===================================================================
> --- libavcodec/libdirac_libschro.h	(revision 13233)
> +++ libavcodec/libdirac_libschro.h	(working copy)
> @@ -80,6 +80,8 @@
>      FfmpegDiracSchroQueueElement *p_head;
>      /** Pointer to tail of queue */
>      FfmpegDiracSchroQueueElement *p_tail;

> +    /** Queue size*/
> +    int32_t size;

I dont think this needs to be exactly 32bit, a int should do.

[...]
> Index: libavcodec/dirac_parser.c
> ===================================================================
> --- libavcodec/dirac_parser.c	(revision 13233)
> +++ libavcodec/dirac_parser.c	(working copy)
> @@ -62,16 +62,12 @@
>      ParseContext *pc = s->priv_data;
>      int next;
>  
> -    if (s->flags & PARSER_FLAG_COMPLETE_FRAMES) {
> -        next = buf_size;
> -    }else{
> -        next = find_frame_end(pc, buf, buf_size);
> +    next = find_frame_end(pc, buf, buf_size);
>  
> -        if (ff_combine_frame(pc, next, &buf, &buf_size) < 0) {
> -            *poutbuf = NULL;
> -            *poutbuf_size = 0;
> -            return buf_size;
> -        }
> +    if (ff_combine_frame(pc, next, &buf, &buf_size) < 0) {
> +        *poutbuf = NULL;
> +        *poutbuf_size = 0;
> +        return buf_size;
>      }
>  
>      *poutbuf = buf;

The current code in dirac_parser.c looks correct to me, this change should
not be needed.
PARSER_FLAG_COMPLETE_FRAMES is supposed to mean "complete" in the ffmpeg
sense.


[...]
> @@ -252,9 +258,10 @@
>      SchroEncoder *encoder = p_schro_params->encoder;
>      struct FfmpegDiracSchroEncodedFrame* p_frame_output = NULL;
>      int go = 1;
> -    SchroBuffer *enc_buf;
> +    SchroBuffer *schro_buf;
>      int presentation_frame;
>      int parse_code;

cosmetic changes must be in seperate patches from functional changes.


[...]

> +            if (p_schro_params->enc_buf_size == 0)
> +                p_schro_params->enc_buf = av_malloc(schro_buf->length);
> +            else {
> +                p_schro_params->enc_buf = av_realloc (
> +                             p_schro_params->enc_buf,
> +                             p_schro_params->enc_buf_size + schro_buf->length
> +                             );
> +            }

This is the same as just
p_schro_params->enc_buf = av_realloc (
        p_schro_params->enc_buf,
        p_schro_params->enc_buf_size + schro_buf->length
        );



> +            memcpy(p_schro_params->enc_buf+p_schro_params->enc_buf_size,
> +                   schro_buf->data, schro_buf->length);
> +            p_schro_params->enc_buf_size += schro_buf->length;
> +
> +

> +            if (state == SCHRO_STATE_END_OF_STREAM) {
> +                p_schro_params->eos_pulled = 1;
> +                   go = 0;
> +            }

the
    indention
        looks
a little odd


[...]
>                  p_frame_output->key_frame = 1;
>              }
> -
>              /* Parse the coded frame number from the bitstream. Bytes 14
>               * through 17 represesent the frame number. */

cosmetic


> -            if (SCHRO_PARSE_CODE_IS_PICTURE(parse_code))
> -            {
> -                assert (enc_buf->length >= 17);

> -                p_frame_output->frame_num = (enc_buf->data[13] << 24) +
> -                                            (enc_buf->data[14] << 16) +
> -                                            (enc_buf->data[15] <<  8) +
> -                                             enc_buf->data[16];
> -            }
> +            p_frame_output->frame_num = (schro_buf->data[13] << 24) +
> +                                        (schro_buf->data[14] << 16) +
> +                                        (schro_buf->data[15] <<  8) +
> +                                         schro_buf->data[16];

reindention counts as cosmetic as well
so yes the if&assert should be removed and the indention then fixed in a
seperate patch, this keeps svnlog and patches more human readable.


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080523/642269f9/attachment.pgp>