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

[Benoit Fouet]

Hi,

(I'll try to give a look at v4 today)

On Sat, 5 Feb 2011 10:36:11 -0800 Philip Langdale wrote:
> 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.
>  

cool

> > > +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?
>  

already answered by Reimar

> > 
> > 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.
>  

I'll have a look in the v4, and see if my mind was clear enough last
time I saw the code :)

> > > +    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.
>  

The first one is a boolean, the second one is not, and if
VDEC_FLAG_BOTTOM_FIRST could not be expressed with a 8-bits variable,
the bottom_first could be wrong.

> > > +    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.
>  

Maybe I was unclear, but that was exactly my point: hardcoding the
pixel format should be enough. Making the width depend on the pixel
format would allow it to change "automagically" if/when the pixel
format changes.

> 
> > > +        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.
>  

OK

> > > +                /*
> > > +                 * 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.
>  

I don't have the knowledge to help on this one, you'll have to see with
the experts :) (this is also true for the rest of the review anyway).

> > > +                 * 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.
>  

Well, IMHO, it should not be done in the decoder. If you have no frame,
fine, just make the caller come back later. Again, this is just my
opinion, you should wait for real advice here.

> > > +        /*
> > > +         * 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.
>  

Well, if you can and it works, I'd say go for it.

> > > +    } 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.
> 

I'd like to hear from other people about those waits.

Thanks,
-- 
Ben