[FFmpeg-devel] [PATCH] H.264/AVCHD interlaced fixes
Michael Niedermayer
michaelni
Mon Feb 16 01:10:05 CET 2009
On Fri, Feb 13, 2009 at 12:32:56PM +0100, Ivan Schreter wrote:
> Michael Niedermayer wrote:
> > On Tue, Feb 10, 2009 at 09:36:35PM +0100, Ivan Schreter wrote:
[...]
> >
> >
> >> + if (ptr==NULL || dst_length < 0)
> >> + break;
> >> + while(ptr[dst_length - 1] == 0 && dst_length > 0)
> >> + dst_length--;
> >> + bit_length= !dst_length ? 0 : (8*dst_length - ff_h264_decode_rbsp_trailing(h, ptr + dst_length - 1));
> >> +
> >> + init_get_bits(&h->s.gb, ptr, bit_length);
> >> + switch(h->nal_unit_type){
> >> + case NAL_SPS:
> >> + ff_h264_decode_seq_parameter_set(h);
> >> + break;
> >> + case NAL_PPS:
> >> + ff_h264_decode_picture_parameter_set(h, h->s.gb.size_in_bits);
> >> + break;
> >> + case NAL_SEI:
> >> + ff_h264_decode_sei(h);
> >> + break;
> >> + case NAL_IDR_SLICE:
> >> + case NAL_SLICE:
> >> + get_ue_golomb(&h->s.gb); // skip first_mb_in_slice
> >> + slice_type = get_ue_golomb_31(&h->s.gb) % 5;
> >> + *ppict_type= golomb_to_pict_type[slice_type];
> >> + *precovery_frame_cnt = h->sei_recovery_frame_cnt;
> >> + if (h->nal_unit_type == NAL_IDR_SLICE) {
> >> + /* this is an IDR key frame (mostly at begin of the stream) */
> >> + *pkey_frame = 1;
> >> + } else if (h->sei_recovery_frame_cnt >= 0) {
> >> + /* key frame, since recovery_frame_cnt is set */
> >> + *pkey_frame = 1;
> >> + }
> >> + pps_id= get_ue_golomb(&h->s.gb);
> >> + if(pps_id>=MAX_PPS_COUNT){
> >> + av_log(h->s.avctx, AV_LOG_ERROR, "pps_id out of range\n");
> >> + return -1;
> >> + }
> >> + if(!h->pps_buffers[pps_id]) {
> >> + av_log(h->s.avctx, AV_LOG_ERROR, "non-existing PPS referenced\n");
> >> + return -1;
> >> + }
> >> + h->pps= *h->pps_buffers[pps_id];
> >> + if(!h->sps_buffers[h->pps.sps_id]) {
> >> + av_log(h->s.avctx, AV_LOG_ERROR, "non-existing SPS referenced\n");
> >> + return -1;
> >> + }
> >> + h->sps = *h->sps_buffers[h->pps.sps_id];
> >> + *pframe_num = get_bits(&h->s.gb, h->sps.log2_max_frame_num);
> >> + if (h->sps.frame_mbs_only_flag) {
> >> + /* single picture for a frame */
> >> + *pfield_pict = 0;
> >> + } else {
> >> + if (get_bits1(&h->s.gb)) { /* field_pict_flag */
> >> + /*
> >> + * This frame is divided in two separate field pictures.
> >> + * Field parity bit follows in picture header, but we
> >> + * don't need it.
> >> + */
> >> + *pfield_pict = 1;
> >> + } else {
> >> + *pfield_pict = 0;
> >> + }
> >> + }
> >>
> >
> > split the patch please so that
> > IDR-keyframe
> > SEI recovery
> > field pics
> >
> > are in 3 seperate patches
> > the original patch i wrote was not such an intermingled mix
> >
> This is no intermingled mix. In the end, it would look exactly like
> this, whether in 3 patches or in 1 patch. It is an integral
its easier to review and test the 3 patches and easier to debug
if bugs arise as there are intermediate steps in svn that one can checkout
and test
> functionality (well, except for field pic, since this is still unused).
> I don't see the reason to split it (maybe except field pict flag / frame
> number, since this is unused yet), since this will obscure the intent of
> the patch - namely communicating key frames (independent of IDR or
> recovery point).
>
> > [...]
> >
> >> +/**
> >> + * Set context parameters for lavf.
> >> + *
> >> + * @param s parser context.
> >> + * @param pict_type picture type of the first slice in frame.
> >> + * @param field_pict set to 1 for field picture, 0 for frame picture.
> >> + * @param key_frame set to 1 for key pictures, 0 for non-key pictures.
> >> + * @param recovery_frame_cnt frame count from SEI recovery point if present
> >> + * or -1 otherwise.
> >> + * @param frame_num frame number.
> >> + */
> >> +static inline void h264_set_keyframe(AVCodecParserContext *s,
> >> + int pict_type,
> >> + int field_pict,
> >> + int key_frame,
> >> + int recovery_frame_cnt,
> >> + int frame_num)
> >> +{
> >> + /*
> >> + * NOTE: Currently, there is no flag to tell libavformat about
> >> + * key frames. Instead, it relies on having pict_type set to I
> >> + * for key frames, so we use this to communicate it. This should
> >>
> >
> > if your patch contains a hack like this its rejected.
> > if theres a bug it has to be fixed, not worked around by such hacks
> >
> I agree with you that bugs have to be fixed. However, I can not and will
> not, again, CAN NOT AND WILL NOT, fix all FFmpeg bugs and design errors,
> which are affecting my patches. Sometimes, there is time for a
> workaround, before other code is fixed (there are plenty in FFmpeg).
> Fixing this particular problem would entail adding two new fields (key
> frame and recovery count) to AVCodecParserContext, removing current key
> frame hack in compute_pkt_fields() and changing all format parsers to
> set key frame field, which is a lot of work and given current practices
> extremely high effort to box it through. I simply don't have time for
> that. I just wanted to help to support AVCHD camcorders, since this is
> my use case (and of a lot of other people willing to use AVCHD
> camcorders with Linux).
>
> Your own code is full of FIXME comments and you don't have a problem
> with it. Why do you have a problem with it when someone else writes a
> patch which does the correct thing but due to deficiencies in other
> components needs to do a workaround? You are using different quality
> measurements for your own and foreign code, which I don't find OK.
i use the same measurements for all code that goes in, but there is historic
code that was not subject to that _AND_ there is code that went in where
only afterwards we realized that it had unexpected problems ...
the I_TYPE == keyframe issue is one of these, it simply predates h.264
>
> Don't get me wrong, in general, I agree with you to minimize
> workarounds/hacks in the code, preferably to zero. I also looked at SVN
> log messages and patch sizes in files you maintain and I find it very
> good to have them so cleanly separated. But if you want to have
> progress, you'll have to accept *some* workarounds (which you do in your
> own code).
i am not short on patches that need a review thus being "too picky"
is not a problem we have in general.
and especially for h.264 we need a correct solution no hacks, this stuff
will contain enough bugs and issues on its own even if we try our best
and i bet you would need less time fixing the leyframe/i type thing than
discussing why its better not to.
>
> Since I'm feeling extremely unwelcome by FFmpeg developer community and
> lack of constructive criticism which spans more than a few words, I'm
> strongly considering to stop all my FFmpeg-related activities, make
> private patches that work for me and maintain them for me only (and
> eventually other people interested).
>
> Please think about it, before scaring yet another promising dev
> community member away.
if something i said sounds rude then iam sorry and i dont want to loose any
developer, but i repeat i cannot accept suboptimal code, you yourself
have said there are design issues and bugs in ffmpeg, well i dont want to
add more ...
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/20090216/d3ca7440/attachment.pgp>
More information about the ffmpeg-devel
mailing list