[FFmpeg-devel] [PATCH]pes packetizer

[Michael Niedermayer]

Hi

On Fri, Aug 31, 2007 at 08:11:33PM +0100, M?ns Rullg?rd wrote:
> Michael Niedermayer <michaelni at gmx.at> writes:
> 
> >>  }
> >>  
> >> -static inline void put_timestamp(ByteIOContext *pb, int id, int64_t timestamp)
> >> +inline void put_timestamp(ByteIOContext *pb, int id, int64_t timestamp)
> >>  {
> >
> > this lacks a ff_ prefix now, which would make the code after this patch
> > broken so we cannot apply it, svn must always be working
> > commiting broken code with the intent to correct it at some unspecified
> > point in the future is not good
> > its better not to break it in the first place
> 
> That said, the reviewing process might be easier if the changes are
> staged, even if some intermediate stages are not fit for SVN.

yes
but in that case all patches needed to reach a working point again should be
posted not just the first of a series


> 
> >>  typedef struct {
> >> -    AVFifoBuffer fifo;
> >> +    PESStream pes_stream;
> >>      uint8_t id;
> >> -    int max_buffer_size; /* in bytes */
> >> -    int buffer_index;
> >> -    PacketDesc *predecode_packet;
> >> -    PacketDesc *premux_packet;
> >> -    PacketDesc **next_packet;
> >>      int packet_number;
> >>      uint8_t lpcm_header[3];
> >>      int lpcm_align;
> >
> > is the lpcm stuff mpeg-PS specific? if not then it does belong in
> > the PES stuff
> 
> The PCM stuff is DVD-specific.  IMHO extensions to the MPEG formats
> like DVD and DVB should be explicitly marked as extensions and only
> enabled when specifically requested.  We'd probably need a new way of
> signalling this information though.

yes agree, though for the moment i think we can leave it as it was
in the patch


> 
> >> +/**
> >> + * muxer type for PES
> >> + */
> >> +typedef enum {
> >> +    PESMUXER_PS,
> >> +    PESMUXER_TS,
> >> +    PESMUXER_PES
> >> +} PESMuxerType;
> >> +
> >
> > the PES muxer should not need to know in what container the PES stream
> > will be embeded
> 
> There are a few subtle differences.  For instance, video PES packets
> with unspecified (coded zero) length are allowed in TS.

do these make sense for us? or can we just make a PES stream which works
in PS as well as TS?


> 
> >> +/**
> >> + * Insert a timestamp into the ByteIOContext.
> >> + * @param[in] pb        the ByteIOContext to be written to
> >> + * @param[in] id        stream ID
> >> + * @param[in] timestamp the timestamp
> >> + * @return  NULL
> >> + */
> >> +inline void put_timestamp(ByteIOContext *pb, int id, int64_t timestamp);
> >
> > this wont compile on a non gnu c compiler
> 
> It will compile in some compilers other than gcc.  The problem is that
> what the compiler does with it is somewhat erratic, even between gcc
> versions.

true, but either way its not acceptable as is. the inline should be removed


> 
> > also i think it would be cleaner to split the PES muxer into a proper
> > AVOutputFormat
> 
> Isn't the PES packetiser just an intermediate stage common to MPEG PS
> and TS?  How will the PS and TS muxers make use of it if it is a full
> AVOutputFormat?

call AVOutputFormat.write_packet() instead of ff_pes_blah()
anyway this was just an idea, if its feasable, maybe its not


btw, do you want to take over the mpegts muxer reviews? :)
i would be very happy if you did, you know more about mpeg-ps/ts than
i do

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

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- 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/20070831/5f952211/attachment.pgp>