[FFmpeg-devel] [PATCH] CrystalHD decoder support v3

Philip Langdale philipl
Sat Feb 5 19:36:11 CET 2011


On Tue, 1 Feb 2011 15:17:37 +0100
Benoit Fouet <benoit.fouet at free.fr> wrote:

> Hi,
> 
> mostly a naive pass on the code.

Ok, I've addressed all the comments that can be addressed by code
changes and pushed those to my github repo.

I'm responding inline to the remaining comments.

> 
> Do you think some code could be shared between this and and the mp4
> to annex B bitstream filter?

What I've checked in does the extradata copy and swap to work with the
unmodified bsf.
 
> > +static inline void print_frame_info(CHDContext *priv,
> > BC_DTS_PROC_OUT *output) +{
> > +    av_log(priv->avctx, AV_LOG_VERBOSE, "\tYBuffSz: %u\n",
> > output->YbuffSz);
> > +    av_log(priv->avctx, AV_LOG_VERBOSE, "\tYBuffDoneSz: %u\n",
> > +           output->YBuffDoneSz);
> > +    av_log(priv->avctx, AV_LOG_VERBOSE, "\tUVBuffDoneSz: %u\n",
> > +           output->UVBuffDoneSz);
> > +    av_log(priv->avctx, AV_LOG_VERBOSE, "\tTimestamp: %lu\n",
> > +           output->PicInfo.timeStamp);
> 
> timestamp is an uint64_t

Yeah, and when compiling 64bit, %lu is correct. For 32bit, you need %llu
which I assume if what you were referring to. Is there a pre-existing
macro substitution in the codebase anywhere?
 
> 
> Why do you need the special case for the head element?
> It seems this should work fine with a 'while (node)' loop, no?

I've added a comment, but it's because its looking one node ahead
except when dealing with the head node, and it has to do that so
that it can rewrite the next pointer in the preceding node.
 
> > +    uint8_t bottom_field = (output->PicInfo.flags &
> > VDEC_FLAG_BOTTOMFIELD) ==
> > +                           VDEC_FLAG_BOTTOMFIELD;
> > +    uint8_t bottom_first = output->PicInfo.flags &
> > VDEC_FLAG_BOTTOM_FIRST; +
> 
> The flags are 16 bits in the lib (even though those ones are 8 bits),
> why do you force those to be 8 bits?

I've changed the code to make it explicit but these are boolean results,
not raw flags.
 
> > +    int width = output->PicInfo.width * 2; // 16bits per pixel
> 
> Even though it's hardcoded for now, this could maybe depend on the
> pixel format?

It does, but the pixel format is hardcoded too, so it's all the same.
I've added a comment.
 

> > +        if (output.PicInfo.height == 1088)
> > +            avctx->height = 1080;
> 
> why this special case? please add a comment.

This came from the xbmc crystalhd code, but there's no particular
reason for it - I think they were trying to be friendly to
mis-encoded files. I've removed it.
 
> > +                /*
> > +                 * Have we lost frames? If so, we need to shrink
> > the
> > +                 * pipeline length appropriately.
> > +                 */
> 
> and nothing is done to do that?

I don't know what to do. I've never seen this case so I'm not sure what
really happens. In reality, the main problem is in mplayer where the
pts for the missing frames will be queued up and I see no way to tell it
to discard them. If I reduce has_b_frames, I think it will shrink from
the wrong end.
 
> > +                 * reorded_opaque value is in ms seems to work.
> > +                 */
> > +                uint64_t pts = opaque_list_push(priv,
> > avctx->reordered_opaque);
> > +                av_log(priv->avctx, AV_LOG_VERBOSE, "input
> > \"pts\": %lu\n",
> > +                       pts);
> > +                ret = DtsProcInput(dev, avpkt->data, len, pts, 0);
> > +                if (ret == BC_STS_BUSY) {
> > +                    av_log(avctx, AV_LOG_WARNING,
> > +                           "CrystalHD: ProcInput returned busy\n");
> > +                    usleep(10000);
> 
> Why are you sleeping here? Is there going to be a retry afterwards
> that you're trying to prevent?

I'm trying to avoid a tight loop where BUSY is returned many many times
before the state changes - which is what happens.
 
> > +        /*
> > +         * No frames ready. Don't try to extract.
> > +         */
> > +        if (decoder_status.ReadyListCount == 0) {
> > +            usleep(50000);
> > +        } else {
> > +            break;
> > +        }
> > +    } while (input_full == 1);
> 
> Isn't there another way to know when a frame is ready than trial and
> error?

In theory, the ReadyListCount is how you know - although even then i've
seen ProcOutput fail with the count != 0. In general, it's all hope for
the best with this hardware. Mind you, I think I can get rid of the
input_full infrastructure - that dates back to before I was able to get
the pipeline depth under control, and now it should never overflow.
 
> > +    } else if (rec_ret == RET_COPY_AGAIN) {
> > +        /*
> > +         * This means we got a FMT_CHANGE event and no frame, so
> > go around
> > +         * again to get the frame.
> > +         */
> > +        receive_frame(avctx, data, data_size, 0);
> 
> No check on the error code this time? what if that fails too?

>From the outside, success or failure is judged by whether any data is
returned. There's nothing to be done in the decoder at this point based
on the error codes.
 
> > +    }
> > +    usleep(10000);
> 
> This sleep, particularly, looks weird. We're returning to the caller,
> but wait before doing it. Why?

This is to reduce the chances of the next decode call happening before
the next frame is ready and having to wait at that point. I guess it's
all effectively the same. I still need to do more tuning on these waits
and when they occur.

Thanks,

--phil



More information about the ffmpeg-devel mailing list