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

[Michael Niedermayer]

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


[...]
> @@ -66,6 +66,7 @@ typedef struct GXFStreamContext {
>  typedef struct GXFContext {
>      AVClass *av_class;
>      uint32_t nb_fields;
> +    _Bool progressive;

what is _Bool ?


>      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


[...]

> +                    } /* 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 ?


[...]
> @@ -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


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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/20130703/7c4c1dca/attachment.asc>