[FFmpeg-devel] [RFC] GXF Patches - HD

[Michael Niedermayer]

On Tue, Jul 02, 2013 at 11:21:57PM -0500, Reuben Martin wrote:
> On Wednesday, July 03, 2013 12:51:53 AM Michael Niedermayer wrote:
> > On Mon, Jun 17, 2013 at 03:26:18PM -0500, Reuben Martin wrote:
> > > HD support (720p and 1080i) & test updates
> > > 
> > > Note: this patch might be a bit big. Suggestions on how to chop it up (if
> > > needed) are welcome.
> > 
> > the most obvious is seperatig reindentions out in a seperate patch
> > 
> > also the checksum updates should be in the patch that causes them
> 
> Ok.
> 

> > 
> > 
> > [...]
> > 
> > > @@ -66,6 +66,7 @@ typedef struct GXFStreamContext {
> > > 
> > >  typedef struct GXFContext {
> > >  
> > >      AVClass *av_class;
> > >      uint32_t nb_fields;
> > > 
> > > +    _Bool progressive;
> > 
> > what is _Bool ?
> 
> It's a C99 boolean data type. "_Bool" was used instead of "bool" because the 
> former keyword was reserved in earlier standards even though no actual boolean 
> type existed. (That's all second-hand information. I'm not a C syntax 
> historian.)
> 
> If int is more appropriate, I can change it to that.

I think int will cause fewer problems


> 
> > 
> > >      uint16_t audio_tracks;
> > >      uint16_t mpeg_tracks;
> > >      int64_t creation_time;
> > > 
> > > @@ -75,7 +76,7 @@ typedef struct GXFContext {
> > > 
> > >      uint32_t umf_length;
> > >      uint16_t umf_track_size;
> > >      uint16_t umf_media_size;
> > > 
> > > -    AVRational time_base;
> > > +    AVRational time_base;     ///< timebase is for fields not frames.
> > > Even with progressive content!> 
> > >      int flags;
> > >      GXFStreamContext timecode_track;
> > >      unsigned *flt_entries;    ///< offsets of packets /1024, starts after
> > >      2nd video field
> > could be in a seperate patch if you like
> > 
> 
> Ok
> 
> > [...]
> > 
> > > +                    } /* else if (fabs(av_q2d(st->codec->time_base) -
> > > 1/30.0) == 0.0) { +                        sc->frame_rate_index = 4;
> > > +                        sc->sample_rate = 30;
> > > +                        gxf->time_base = (AVRational){ 1, 60 };
> > > +                        gxf->flags |= 0x00000400;
> > > +                        sc->fields = 1;
> > > +                        gxf->progressive = 1;
> > > +                    } else if (fabs(av_q2d(st->codec->time_base) -
> > > 1001/30000.0) < 0.0001) { +                        sc->frame_rate_index =
> > > 5;
> > > +                        sc->sample_rate = 30;
> > > +                        gxf->time_base = (AVRational){ 1001, 60000 };
> > > +                        gxf->flags |= 0x00000400;
> > > +                        sc->fields = 1;
> > > +                        gxf->progressive = 1;
> > > +                    } else if (fabs(av_q2d(st->codec->time_base) -
> > > 1/25.0) < 0.0001) { +                        sc->frame_rate_index = 6;
> > > +                        sc->sample_rate = 25;
> > > +                        gxf->time_base = (AVRational){ 1, 50 };
> > > +                        gxf->flags |= 0x00000200;
> > > +                        sc->fields = 1;
> > > +                        gxf->progressive = 1;
> > > +                    } else if (fabs(av_q2d(st->codec->time_base) -
> > > 1/24.0) == 0.0) { +                        sc->frame_rate_index = 7;
> > > +                        sc->sample_rate = 24;
> > > +                        gxf->time_base = (AVRational){ 1, 48 };
> > > +                        gxf->flags |= 0x00000100;
> > > +                        sc->fields = 1;
> > > +                        gxf->progressive = 1;
> > > +                    } else if (fabs(av_q2d(st->codec->time_base) -
> > > 1001/24000.0) < 0.0001) { +                        sc->frame_rate_index =
> > > 8;
> > > +                        sc->sample_rate = 24;
> > > +                        gxf->time_base = (AVRational){ 1001, 48000 };
> > > +                        gxf->flags |= 0x00000100;
> > > +                        sc->fields = 1;
> > > +                        gxf->progressive = 1;
> > > +                    } else {
> > > +                        av_log(s, AV_LOG_ERROR, "Invalid frame rate for
> > > 1080i or 1080p content\n"); +                        return -1;
> > > +                    }*/
> > 
> > Why is this code commented out but in the patch ?
> 
> It's not intended to be committed that way, it is more for the sake of 
> feedback. I'm not sure how to deal with interlaced vs progressive. In GXF 
> everything is field based. For instance a 50 fields per second (interlaced) 
> video stream is tagged differently than 25 frames per second (progressive) 
> video stream. But in ffmpeg, it seems that everything is treated as frames. So 
> 50i and 25p streams would both return a timebase of 25/1. It seems that all  
> handling of interlacing is left to be delt with in the context of the 
> individual codecs.
> 
> At least from what I can deduce. I might be totally wrong due to not being 
> very familiar with the API. If there is a way to query if the video stream is 
> field based or not, the " _Bool progressive" value in the GXFContext struct 
> would go away. Getting rid of that variable would be ideal, because that 
> variable is setting progressive / interlaced in the format context rather than 
> the individual stream context. This would have the potentual to cause problems 
> in the case where there is more than one video stream, with on being 
> progressive and another being interlaced.

You could try AVCodecContext.field_order
but iam not sure how well this would work

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130704/a94ee0e3/attachment.asc>