[FFmpeg-devel] [RFC] - GXF Encoder fixes and HD support

[Carl Eugen Hoyos]

Reuben Martin <reuben.m <at> gmail.com> writes:

> Comments, suggestions, code, flames all welcome.

I am sure work on this is very welcome, I can't really comment on the details,
but here are a few cosmetic hints:

Please split the short patch again: I suspect the two hunks are unrelated (could
you confirm that you can produce valid H264 gxf files only with this short
change? If not, it should be part of one patch that adds H264 support.)

> -    //{ CODEC_ID_NONE,  ,   18 }, /* Non compressed 24 bit audio */
> +    //{ CODEC_ID_NONE    ,  18 }, /* Non compressed 24 bit audio */

> -                gxf->flags |= 0x00000040;
>                  gxf->time_base = (AVRational){ 1, 50 };
> +                gxf->flags |= 0x00000040;

Such changes are very unwelcome, please remove them before submitting (they make
reviewing harder).

> -                    sc->media_type += 2;
> -                    sc->track_type = 6;
> -                    gxf->flags |= 0x00002000;
> -                    media_info = 'E';
...
> +                        sc->media_type += 2;
> +                        gxf->flags |= 0x00002000;
> +                        media_info = 'E';
> +                        sc->track_type = 6;

Please do not re-indent in this case to make the patch smaller. (Re-indentation
goes into a separate commit.)

> +#if 0
> +            case CODEC_ID_H264:
> +                    sc->media_type = 26;
> +                    gxf->flags |=  0x02000000;
> +                    media_info = 'I';
> +                    sc->track_type = 11;
> +                break;
> +#endif

If this does not work, please remove it (including the hunk in the other file),
dead code often just rots.

I don't know much about gxf, but if you try to fix different issues here (it
looks as if), please try to send one patch per issue (not per file).

Thank you, Carl Eugen