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

[Reuben Martin]

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.

> 
> >      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.


> 
> 
> [...]
> 
> > @@ -871,6 +1067,7 @@ static int gxf_write_header(AVFormatContext *s)
> > 
> >      gxf_write_flt_packet(s);
> >      gxf_write_umf_packet(s);
> > 
> > +
> > 
> >      gxf->packet_count = 3;
> >      
> >      avio_flush(pb);
> 
> doesnt belong in the patch
> 

OK

> > @@ -986,8 +1183,11 @@ static int gxf_write_packet(AVFormatContext *s,
> > AVPacket *pkt)> 
> >      int packet_start_offset = avio_tell(pb) / 1024;
> >      
> >      gxf_write_packet_header(pb, PKT_MEDIA);
> > 
> > -    if (st->codec->codec_id == AV_CODEC_ID_MPEG2VIDEO && pkt->size % 4)
> > /* MPEG-2 frames must be padded */ -        padding = 4 - pkt->size % 4;
> > +    if (st->codec->codec_id == AV_CODEC_ID_MPEG2VIDEO) /* MPEG-2 frames
> > must be padded */ +        if (st->codec->height >= 720 && pkt->size %
> > 16)
> > +            padding = 16 - pkt->size % 16;
> > +        else if (st->codec->height < 720 && pkt->size % 4)
> > +            padding = 4 - pkt->size % 4;
> > 
> >      else if (st->codec->codec_type == AVMEDIA_TYPE_AUDIO)
> 
> nested if/else should use {} its too easy to add bugs otherwise
> 

Ok.

-Reuben