[MPlayer-dev-eng] [PATCH]VO_VDPAU, round 5
Reimar Döffinger
Reimar.Doeffinger at stud.uni-karlsruhe.de
Thu Jan 22 16:40:53 CET 2009
Hello,
On Thu, Jan 22, 2009 at 03:34:27AM +0100, Carl Eugen Hoyos wrote:
> +static int visible_buf = -1; // -1 means: no buffer was drawn yet
> +static uint32_t max_width = 0, max_height = 0; // zero means: not set
Should be initialized in config() and only in config, besides an
explicit initialization to 0 is pointless, it is implicit.
> +static void calc_drwXY_dWH(uint32_t *drwX, uint32_t *drwY, uint32_t *d_wh, uint32_t *d_ht)
> +{
> + vo_calc_drwXY(drwX, drwY);
> +
> + outRectVid.x0 = *drwX;
> + outRectVid.y0 = *drwY;
> + outRectVid.x1 = vo_dwidth+outRectVid.x0;
> + outRectVid.y1 = vo_dheight+outRectVid.y0;
> +
> + outRect.x0 = 0;
> + outRect.x1 = FFMAX(vo_screenwidth, outRectVid.x1);
> + outRect.y0 = 0;
> + outRect.y1 = FFMAX(vo_screenheight, outRectVid.y1);
Are these FFMAX indeed necessary? What for exactly? vo_screenwidth may be
unreliable. How does this handle videos larger than the screen
resolution (in non-fullscreen mode, so the video "hangs out of the
screen"?
> + struct vdp_function {
> + const int id;
> + void * pointer;
> + };
> +
> + const struct vdp_function * dsc;
IMO either always put a space after the * or never.
Also, if you use a int loop variable i and use vdp_func[i].whatever
you would not need that pointer and would not have to name the struct.
If that is nicer or uglier is a matter of taste.
> + vdp_st = vdp_device_create_x11(mDisplay, mScreen, &vdp_device, &vdp_get_proc_address);
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
I imagine saying it before: if these error checks are always the same,
make them a macro.
But since there (I hope) is only one vdp_device_create_x11 call, the
error should just say that creating the device failed.
Also, why does this not abort? Won't it just crash later on when this
fails? And shouldn't it be MSGL_FATAL then?
> + for (dsc = vdp_func; dsc->pointer; dsc++) {
> + vdp_st = vdp_get_proc_address(vdp_device, dsc->id, dsc->pointer);
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
But e.g. this is a case where a _proper_ error messages is necessary,
like this it is completely useless, at the very least you'd need to
print a function name to make it useful.
> + vdp_st = vdp_device_destroy(vdp_device);
> + if (vdp_st != VDP_STATUS_OK) {
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
Adding a proper and understandable message should be no problem.
Also I can't imagine this will cause playback problems, so it would be
MSGL_WARN at most, more likely MSGL_V...
> + vdp_st = vdp_presentation_queue_target_create_x11(vdp_device, vo_window, &vdp_flip_target);
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
> +
> + vdp_st = vdp_presentation_queue_create(vdp_device, vdp_flip_target, &vdp_flip_queue);
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
Won't the second call crash if the first fails? Also human-readable
error messages should be easy here, too (Just something like "Error
creating presentation queue: %i\n").
> + vdp_st = vdp_presentation_queue_destroy(vdp_flip_queue);
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
> +
> + vdp_st = vdp_presentation_queue_target_destroy(vdp_flip_target);
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
As above, readable messages and MSGL_WARN at most.
> + if (vo_config_count)
> + return 0;
> +
> + image_format = format;
You _must_ set image_format and all the other stuff always, try playing
two different files (e.g. one H.264, one VC-1) and -fixed-vo.
> + default:
> + assert(0);
Asserts might not be compiled in in release builds, why not just return
-1?
> + /* -----VDPAU related code here -------- */
> + if (num_video_surfaces)
> + videoSurfaces = calloc(num_video_surfaces, sizeof(VdpVideoSurface));
You will also have to think how to handle this on multiple config()
calls
> + /* get proc address */
> + win_x11_init_vdpau_procs();
> + win_x11_init_vdpau_flip_queue();
Why is this in config() and not in preinit()? If these fail, returning an
error would allow MPlayer to select a different vo (probably not too
helpful in this case though).
> + for (j = 0; j < w; j++) {
> + indexData[i*2*w + j*2] = src[i*stride+j]; // index for palette-table
> + a = srca[i*stride+j]; // alpha_data
> + indexData[i*2*w + j*2 + 1] = -srca[i*stride+j];;
> + }
a is not used anywhere?
> +static int draw_slice(uint8_t * image[], int stride[], int w, int h,
> + int x, int y)
> +{
> + return VO_TRUE;
> +}
Don't the others return VO_FALSE when it is not implemented?
> +static int draw_frame(uint8_t * src[])
> +{
> + mp_msg(MSGT_VO,MSGL_ERR, MSGTR_LIBVO_XV_DrawFrameCalled);
> + return -1;
> +}
I'd remove the message, I don't think this is likely happen these days.
> + switch (image_format) {
> + case IMGFMT_YV12:
> + assert(mpi->num_planes == 3);
> + vdp_ycbcr_format = VDP_YCBCR_FORMAT_YV12;
> + destdata[0] = mpi->planes[0];
> + destdata[1] = mpi->planes[2];
> + destdata[2] = mpi->planes[1];
> + videoSurface = videoSurfaces[0];
> +
> + vdp_st = vdp_video_surface_put_bits_y_cb_cr(videoSurface,
> + vdp_ycbcr_format, // YV12
> + destdata,
> + mpi->stride); // pitch
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
> + break;
> + default:
> + break;
> + }
I don't really like this, a switch just for two cases seems overkill,
als it is unclear to me if this code just to support output of normal
YV12 data, if so it might make sense to cut the size by removing support
for that at least for now (as long as there is no support for
post-processing it is not really useful anyway).
> + if (!num_video_surfaces) { // RGB surface formats
> + switch (image_format) {
> + case IMGFMT_BGRA:
There is a comparison against the num_video_surfaces variable and one
direct after for IMGFMT_BGRA, and the former one carries the comment
"// RGB surface formats". To me that is always an indication that the
code makes no sense and you are checking for the wrong thing, possibly
out of lazyness.
I suspect what was actually _meant_ was something like
> if (IS_RGB(image_format)) {
> ...
> } else {
> assert(num_video_surfaces);
> ...
> }
and num_video_surfaces was only used for the check because it was
convenient, not because it made any sense at all.
> + switch (image_format) {
> + case IMGFMT_BGRA:
> + assert(mpi->num_planes == 1);
> + vdp_rgba_format = VDP_RGBA_FORMAT_B8G8R8A8;
> + destdata[0] = mpi->planes[0];
> + break;
> + default:
> + assert(0);
And get rid of those pointless switches, we can decide about a clean way
to add more format support when there are more formats to support.
E.g. this one should be
assert(image_format == IMGFMT_BGRA);
vdp_rgba_format = ....
Also the whole
destdata[0] = mpi->planes[0];
should probably be moved out of all these comparisons.
Or even better, just initialize
destdata to {mpi->planes[0], mpi->planes[1], mpi->planes[2]}
and only treat the cases where that does not work in a special way.
> +#if TRACE_SURFACES
> + printf("\DRAW IMG:%u\n", videoSurface);
\D?
> +static void DestroyVdpauObjects()
> +{
> + int i;
> + VdpStatus vdp_st;
> +
> + vdp_st = win_x11_fini_vdpau_flip_queue();
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
> +
> + if (num_video_surfaces) {
> + vdp_st = vdp_video_mixer_destroy(videoMixer);
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
> + }
> +
> + for (i = 0; i < NUM_OUTPUT_SURFACES; i++) {
> + vdp_st = vdp_output_surface_destroy(outputSurfaces[i]);
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
> + }
> +
> + for (i = 0; i < num_video_surfaces; i++) {
> + vdp_st = vdp_video_surface_destroy(videoSurfaces[i]);
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
> + }
> +
> + vdp_st = win_x11_fini_vdpau_procs();
> + if (vdp_st != VDP_STATUS_OK)
> + mp_msg(MSGT_VO, MSGL_ERR, "Error %d at %s:%d\n",
> + vdp_st, __FILE__, __LINE__);
It might make sense to just accumulate all error values and print only
one message, and MSGL_WARN at most here, too, IMHO.
That's all for now.
Greetings,
Reimar Döffinger
More information about the MPlayer-dev-eng
mailing list