[FFmpeg-devel] [PATCH] Patches to fix issue453 in libdiracschroedinger
Michael Niedermayer
michaelni
Fri May 23 22:15:14 CEST 2008
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>
More information about the ffmpeg-devel
mailing list