[vlc-devel] [PATCH]: HEVC/H.265 decoder using libde265

[Joachim Bauch]

Hello,

On 07.05.2014 17:12, Rémi Denis-Courmont wrote:
> contrib and module should be separate patches.

ok.

> Also...

Thanks for your feedback, comments see below.

Best regards,
  Joachim

[...]
> +.sum-libde265: libde265-$(LIBDE265_VERSION).tar.gz
> +    $(warning $@ not implemented)
> 
> WTH is that not implemented?

Will be added.

[...]
> --- a/extras/package/ios/build.sh
> +++ b/extras/package/ios/build.sh
> @@ -224,6 +224,7 @@ echo "EXTRA_CFLAGS += ${EXTRA_CFLAGS}" >> config.mak
>  echo "EXTRA_LDFLAGS += ${EXTRA_LDFLAGS}" >> config.mak
>  make fetch
>  make
> +make .libde265
> 
> I'm not familiar with iOS, but this is dubious.
[...]
> --- a/extras/package/macosx/build.sh
> +++ b/extras/package/macosx/build.sh
> @@ -116,6 +116,7 @@ mkdir -p build && cd build
>  ../bootstrap --build=$TRIPLET --host=$TRIPLET > $out
>  if [ ! -e "../$TRIPLET" ]; then
>      make prebuilt > $out
> +    make .libde265 > $out
> 
> Ditto.

As libde265 is not included in the prebuilt tarball, it has to be
compiled manually.

[...]
> +
> +vlc_module_begin ()
> +    set_shortname("libde265dec")
> 
> Do we assume this is never localized? Not everybody uses the Latin
> alphabet.

Will be changed.

[...]
> +    block_t *block = *pp_block;
> +    if (!block)
> +        return NULL;
> 
> I think pp_block can be NULL to trigger flushing nowadays, but I am not
> sure.

Where can I find information about this? I checked other codecs which
also just return for a NULL block.

[...]
> +    if (!dec->b_pace_control && (sys->late_frames > 0) &&
> +        (mdate() - sys->late_frames_start >
> INT64_C(LATE_FRAMES_DROP_ALWAYS_AGE*1000000))) {
> 
> Use CLOCK_FREQ.

Thanks, will change this.

[...]
> +        int size = __MIN( src_stride, dst_stride );
> +        for( int line = 0; line < pic->p[plane].i_visible_lines; line++
> ) {
> +            memcpy( dst, src, size );
> 
> Ouch... can't this library decode directly to the picture buffers?
> 
> This is especially bad with a precedence higher than libavcvodec's...

Unfortunately not, but we have this on the roadmap for a later version
of the library.

[...]
> +error:
> +    if (*pp_block != NULL) {
> 
> Tautology?

Yes, will change this.

[...]
> +    int threads = __MIN(vlc_GetCPUCount() * 2, MAX_THREAD_COUNT);
> +    if (threads > 1) {
> 
> Seems like a tautology.

Yes, will change this.