[FFmpeg-devel] [PATCH] Patches to fix issue453 in libdiracschroedinger
Anuradha Suraparaju
anuradha
Sun Jun 1 10:03:12 CEST 2008
Hi,
I have addressed most of the issues mentioned in your email in the new
patches.
On Fri, 2008-05-23 at 22:15 +0200, Michael Niedermayer wrote:
> 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.
>
Fixed.
>
> > @@ -112,3 +115,8 @@
> >
> > return NULL;
> > }
> > +
> > +int32_t ff_dirac_schro_queue_size (FfmpegDiracSchroQueue *queue)
> > +{
> > + return queue->size;
> > +}
>
> Senseless wraper function.
>
Fixed.
>
> > 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.
>
Fixed.
> [...]
> > 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.
>
As I mentioned in another email, libschroedinger requires that packet
sent to it be Dirac byte stream parse units. If a packet contains more
than one parse-unit the second gets ignored. Hence the change I made is
required.
>
> [...]
> > @@ -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.
>
Will submit another patch with the cosmetics changes once this patch is
accepted.
>
> [...]
>
> > + 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
> );
>
>
Fixed.
>
> > + 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
>
Fixed.
>
> [...]
> > p_frame_output->key_frame = 1;
> > }
> > -
> > /* Parse the coded frame number from the bitstream. Bytes 14
> > * through 17 represesent the frame number. */
>
> cosmetic
>
Will submit another patch with the cosmetics changes once this patch is
accepted.
>
> > - 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.
>
Removed the modified indentation as requested. Will submit another patch
with the cosmetics changes once this patch is accepted.
>
> [...]
Regards,
Anuradha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: issue453_fix_pts_bug_common_libdirac_libschroedinger_svn_13594.diff
Type: text/x-patch
Size: 2264 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080601/9306030e/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: issue453_fix_pts_bug_libschroedinger_svn_13594.diff
Type: text/x-patch
Size: 5014 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080601/9306030e/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_pts_bug_libdirac_svn_13594.diff
Type: text/x-patch
Size: 4717 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080601/9306030e/attachment-0002.bin>
More information about the ffmpeg-devel
mailing list