[FFmpeg-cvslog] r19196 - trunk/ffmpeg.c

[Michael Niedermayer]

On Tue, Jun 16, 2009 at 06:07:42PM -0700, Baptiste Coudurier wrote:
> Michael Niedermayer wrote:
> > On Tue, Jun 16, 2009 at 02:05:56PM -0700, Baptiste Coudurier wrote:
> >> Hi Michael,
> >>
> >> On 6/16/2009 1:15 PM, Michael Niedermayer wrote:
> >>> On Mon, Jun 15, 2009 at 01:55:25AM +0200, bcoudurier wrote:
> >>>> Author: bcoudurier
> >>>> Date: Mon Jun 15 01:55:25 2009
> >>>> New Revision: 19196
> >>>>
> >>>> Log:
> >>>> do not use av_parser_change for h264 which uses bitstream filter, fix #1027
> >>>>
> >>>> Modified:
> >>>>    trunk/ffmpeg.c
> >>>>
> >>>> Modified: trunk/ffmpeg.c
> >>>> ==============================================================================
> >>>> --- trunk/ffmpeg.c	Mon Jun 15 01:14:56 2009	(r19195)
> >>>> +++ trunk/ffmpeg.c	Mon Jun 15 01:55:25 2009	(r19196)
> >>>> @@ -1407,8 +1407,13 @@ static int output_packet(AVInputStream *
> >>>>                          opkt.flags= pkt->flags;
> >>>>  
> >>>>                          //FIXME remove the following 2 lines they shall be replaced by the bitstream filters
> >>>> +                        if(ost->st->codec->codec_id != CODEC_ID_H264) {
> >>>>                          if(av_parser_change(ist->st->parser, ost->st->codec, &opkt.data, &opkt.size, data_buf, data_size, pkt->flags & PKT_FLAG_KEY))
> >>>>                              opkt.destruct= av_destruct_packet;
> >>>> +                        } else {
> >>>> +                            opkt.data = data_buf;
> >>>> +                            opkt.size = data_size;
> >>>> +                        }
> >>>>  
> >>> did i approve this ?
> >> Are you against ?
> > 
> > yes, the correct fix probably would be to improve the AVParser split API
> > short of that a check for mov vs raw h264 in the h264 split would also do
> > this one really is buggy, as its a workaround in one application,
> > is ffmpeg.c the only app using av_parser_change(), i doubt it
> 
> I think it is the only application using it considering how obscure it
> is. Feel free to revert it and reopen the bug in roundup. I've no
> interest in fixing parser_split which is obsoleted by bitstream filters.

well, the remove extradata bitstream filter calls parser->split()
also av_find_stream_info() calls split(), and there is no bsf that could
replace split() currently, not countig one that calls split()

your change affects just 1 of these 3 ...

also
the idea behind split() was to put the global headers in extradata so
all code comming later could just take it from there instead of having
several places from where to extract it.

Now this can be done by a bsf too and iam not against it but such bsf
would have to be always enabled or code that has been simplified based
on an assumption of such bsf would break ...

This idea is pretty much the same as filling in missing pts/dts, its
just filling in extradata


> 
> > that is, if i guessed correctly which bug this is supposed to fix ...
> 
> parser_change is called unconditionnaly that's wrong in any case.
> The check is up to you, I see no problem with checking against codec id.

iam not concerned about the code in ffmpeg.c but rather that this does
not fix the split() code and that we are not even close to replace split
by bsf, not to mention the check split would need would as well be
needed in a bsf doing splits job.

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

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- 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-cvslog/attachments/20090617/f4c64d0d/attachment.pgp>