On Mon, Mar 21, 2016 at 01:08:32PM +0000, Mark Thompson wrote:
> On 21/03/16 12:29, Diego Biurrun wrote:
> > On Mon, Mar 21, 2016 at 12:04:23AM +0000, Mark Thompson wrote:
> >> --- a/libavcodec/Makefile
> >> +++ b/libavcodec/Makefile
> >> @@ -105,7 +105,7 @@ OBJS-$(CONFIG_STARTCODE) += startcode.o
> >> OBJS-$(CONFIG_TEXTUREDSP) += texturedsp.o
> >> OBJS-$(CONFIG_TEXTUREDSPENC) += texturedspenc.o
> >> OBJS-$(CONFIG_TPELDSP) += tpeldsp.o
> >> -OBJS-$(CONFIG_VAAPI_ENCODE) += vaapi_encode.o
> >> +OBJS-$(CONFIG_VAAPI_ENCODE) += vaapi_encode.o
> >> vaapi_encode_h26x.o
> >
> > Is vaapi_encode_h26x.o required by your MJPEG encoder as well? If not,
> > then this object file should not be compiled for it. You can add
> > another vaapi_encode_h26x component that conditionally enables it.
>
> It only applies to H.264 and H.265.
>
> Would it be better to instead add the vaapi_encode_h26x.o object to the lists
> for both of the other encoders?
That would avoid bloating the mjpeg-only build, yes.
IMO it's even cleaner to avoid adding the .o file to both lists and add
another config variable. Feel free to ignore this suggestion if you
prefer; I'll probably change it myself later on then.
> >> --- /dev/null
> >> +++ b/libavcodec/vaapi_encode_h264.c
> >> @@ -0,0 +1,817 @@
> >> +
> >> +typedef struct {
> >> +} VAAPIEncodeH264MiscSliceParams;
> >
> > Please avoid anonymously typedeffed structs.
>
> You mean just give it a tag, so:
>
> typedef struct VAAPIEncodeH264MiscSliceParams {
> ...
> } VAAPIEncodeH264MiscSliceParams;
>
> ?
Yes.
> >> --- /dev/null
> >> +++ b/libavcodec/vaapi_encode_h26x.h
> >> @@ -0,0 +1,70 @@
> >> +
> >> +// Set to 1 to log a full trace of all bitstream output (debugging only).
> >> +#if 0
> >
> > Ugh, commented out code tends to rot and break... :-/
>
> The trace code is useful for comparison against the decoder while debugging
> it;
> it has little value once the code is all written.
Then you can probably just drop it all and be done with it.
> >> +int ff_vaapi_encode_h26x_nal_unit_to_byte_stream(uint8_t *dst, size_t
> >> *dst_len,
> >> + uint8_t *src, size_t
> >> src_len);
> >
> > This header should directly #include stdint.h and sys/types.h.
>
> Or stdint.h and stddef.h?
Or that, yes.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
