[FFmpeg-devel] [PATCH] Patches to fix issue453 in libdiracschroedinger
Michael Niedermayer
michaelni
Sun Jun 15 02:25:52 CEST 2008
On Sun, Jun 01, 2008 at 06:03:12PM +1000, Anuradha Suraparaju wrote:
> 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.
patch 2 of 3 and 3 of 3 in this email look ok
[...]
--
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/20080615/f286fa16/attachment.pgp>
More information about the ffmpeg-devel
mailing list