[FFmpeg-devel] [PATCH] set bits_per_raw_sample when transcoding into wide colorspace

[Lars Täuber]

On Sat, 5 Sep 2009 05:24:35 +0200 Michael Niedermayer <michaelni at gmx.at> wrote:
> On Fri, Sep 04, 2009 at 10:37:20PM +0200, Lars T?uber wrote:
> > On Fri, 4 Sep 2009 21:20:36 +0200 Michael Niedermayer <michaelni at gmx.at> wrote:
> > > On Fri, Sep 04, 2009 at 03:22:54PM +0200, Lars T?uber wrote:
> > > > $subject
> > > > 
> > > > Lars
> > > 
> > > >  ffmpeg.c |   14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > > 84909e388d819a1c59f18c4d7ee229c144134d81  16bps.diff
> > > > Index: ffmpeg.c
> > > > ===================================================================
> > > > --- ffmpeg.c	(revision 19746)
> > > > +++ ffmpeg.c	(working copy)
> > > > @@ -985,6 +985,20 @@
> > > >          }
> > > >          sws_scale(ost->img_resample_ctx, formatted_picture->data, formatted_picture->linesize,
> > > >                0, ost->resample_height, resampling_dst->data, resampling_dst->linesize);
> > > > +
> > > > +        switch (ost->st->codec->pix_fmt) {
> > > > +        case PIX_FMT_GRAY16BE:
> > > > +        case PIX_FMT_GRAY16LE:
> > > > +        case PIX_FMT_RGB48BE:
> > > > +        case PIX_FMT_RGB48LE:
> > > > +        case PIX_FMT_YUV420PBE:
> > > > +        case PIX_FMT_YUV422PBE:
> > > > +        case PIX_FMT_YUV444PBE:
> > > > +        case PIX_FMT_YUV420PLE:
> > > > +        case PIX_FMT_YUV422PLE:
> > > > +        case PIX_FMT_YUV444PLE:
> > > > +            ost->st->codec->bits_per_raw_sample = 16;
> > > > +        }
> > > 
> > > doing it like that is a sure recipe for forgetting to add a format
> > > in the future.
> > > the pix_fmt_info table might be a better way to check
> > 
> > Ok, what about this?
> 
> as you add a public function (av prefix & in public header) this would
> need some justification why such public function makes sense,

The justification is that the pix_fmt_info is not directly accessible. Therefore a public function is needed. 
There already is the indirect access to the pixfmt name through avcodec_get_pix_fmt inside ffmpeg.c that's why I thought it is the right way to access pix_fmt_info.

Btw. there should be public functions to read the other features of pix_fmt_info too. The other way I see is to move the definition and declaration from imgconvert.c into a header file.

> also it
> would need to be better documented though some doxy should be there
> either way ff_ or av_ ...
> (the doxy also should say a word about rgb16 that have different depth
>  for r/g/b)

I'm not sure if I'm right with the description of the return value for RGB565. It might also be that the value used for the majority of components is returned or the rounded down average or something else I can't think of.
This should also be documented in doxy of the definition of PixFmtInfo I think.
 
> 
> > 
> > BTW isn't this pix_fmt_... stuff not better belonging into libavutil/pixfmt.[ch] ?
> 
> there was some discussion about this on the ML, short awnser is no, for the
> long awnser iam too tired at the moment to even remember it fully

The short answer is always enough to me.

Lars
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 16bps.diff
Type: text/x-diff
Size: 1968 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090905/43cae060/attachment.diff>