nV News Forums VdpVideoSurfaceGet/PutBitsYCbCr() cause invalid reads/writes for frame size 1136x1337
 Home Feed Me Chat Register FAQ Calendar Mark Forums Read

 Newegg Daily Deals

08-29-09, 06:50 PM   #1
rnissl
Registered User

Join Date: Jun 2005
Posts: 36
VdpVideoSurfaceGet/PutBitsYCbCr() cause invalid reads/writes for frame size 1136x1337

Hi,

due to a data corruption in a 1920x1080i MPEG2 stream, the following xine-lib code is called with a frame size of 1136x1337 to copy a frame's image, i. e. to duplicate a frame in terms of xine-lib:
Code:
    this->vo_frame.pitches[0] = 8*((orig->vo_frame.width + 7) / 8);
this->vo_frame.pitches[1] = 8*((orig->vo_frame.width + 15) / 16);
this->vo_frame.pitches[2] = 8*((orig->vo_frame.width + 15) / 16);
this->vo_frame.base[0] = av_mallocz(this->vo_frame.pitches[0] * orig->vo_frame.height);
/*949*/    this->vo_frame.base[1] = av_mallocz(this->vo_frame.pitches[1] * ((orig->vo_frame.height+1)/2));
/*950*/    this->vo_frame.base[2] = av_mallocz(this->vo_frame.pitches[2] * ((orig->vo_frame.height+1)/2));
format = VDP_YCBCR_FORMAT_YV12;

fprintf(stderr, "orig->vo_frame_width: %d, orig->vo_frame.height: %d\n", orig->vo_frame.width, orig->vo_frame.height);
/*959*/  st = vdp_video_surface_getbits_ycbcr(orig->vdpau_accel_data.surface, format, this->vo_frame.base, this->vo_frame.pitches);
if (st != VDP_STATUS_OK)
printf("vo_vdpau: failed to get surface bits !! %s\n", vdp_get_error_string(st));

/*963*/  st = vdp_video_surface_putbits_ycbcr(this->vdpau_accel_data.surface, format, this->vo_frame.base, this->vo_frame.pitches);
if (st != VDP_STATUS_OK)
printf("vo_vdpau: failed to put surface bits !! %s\n", vdp_get_error_string(st));
You can find the report of valgrind --smc-check=all --leak-check=full -v as an attachment.

I assume this to be a bug in the mentioned functions.

Bye.
Attached Files
 valgrind.txt (28.1 KB, 87 views)

 08-31-09, 01:28 PM #2 Stephen Warren Moderator     Join Date: Aug 2005 Posts: 1,327 Re: VdpVideoSurfaceGet/PutBitsYCbCr() cause invalid reads/writes for frame size 1136x Were the VdpVideoSurfaces allocated as 1920x1080? If so, then VDPAU will always read/write that many pixels (which is more pixels than the 1136x1337 malloc'd buffer). The pitches[] array simply lets VDPAU calculate the address within the buffer to write/read, and in no way constrains the number of pixels written in a row, nor the number of rows written. In other words, the size of the get/put-bits buffer must match the size of the VdpVideoSurface exactly.
08-31-09, 04:13 PM   #3
rnissl
Registered User

Join Date: Jun 2005
Posts: 36
Re: VdpVideoSurfaceGet/PutBitsYCbCr() cause invalid reads/writes for frame size 1136x

Hi,

Quote:
 Originally Posted by Stephen Warren Were the VdpVideoSurfaces allocated as 1920x1080? If so, then VDPAU will always read/write that many pixels (which is more pixels than the 1136x1337 malloc'd buffer). The pitches[] array simply lets VDPAU calculate the address within the buffer to write/read, and in no way constrains the number of pixels written in a row, nor the number of rows written. In other words, the size of the get/put-bits buffer must match the size of the VdpVideoSurface exactly.
I had a look at xine-lib's code and do not see any manipulation to width or height values extracted from the bitstream. So the frames are allocated at the size which can be found in the vo_frame.width and .height members and which was logged by a printf in the valgrind output.

I've modified xine-lib to use VdpVideoSurfaceGetParameters() in front of the printf and to report the retrieved values. The code looks like that:
Code:
  if (!(orig->flags & VO_CHROMA_422)) {
this->vo_frame.pitches[0] = 8*((orig->vo_frame.width + 7) / 8);
this->vo_frame.pitches[1] = 8*((orig->vo_frame.width + 15) / 16);
this->vo_frame.pitches[2] = 8*((orig->vo_frame.width + 15) / 16);
this->vo_frame.base[0] = av_mallocz(this->vo_frame.pitches[0] * orig->vo_frame.height);
this->vo_frame.base[1] = av_mallocz(this->vo_frame.pitches[1] * ((orig->vo_frame.height+1)/2));
this->vo_frame.base[2] = av_mallocz(this->vo_frame.pitches[2] * ((orig->vo_frame.height+1)/2));
format = VDP_YCBCR_FORMAT_YV12;
} else {
this->vo_frame.pitches[0] = 8*((orig->vo_frame.width + 3) / 4);
this->vo_frame.base[0] = av_mallocz(this->vo_frame.pitches[0] * orig->vo_frame.height);
format = VDP_YCBCR_FORMAT_YUYV;
}

VdpChromaType ct = (VdpChromaType)-1;
int w = -1;
int h = -1;
st = vdp_video_surface_get_parameters(orig->vdpau_accel_data.surface, &f, &w, &ct);
if (st != VDP_STATUS_OK)
printf("vo_vdpau: failed to get parameters !! %s\n", vdp_get_error_string(st));

fprintf(stderr, "orig->vo_frame_width: %d, orig->vo_frame.height: %d, format: %d, w: %d, h: %d, ct: %d\n", orig->vo_frame.width, orig->vo_frame.height, format, w, h, ct);
st = vdp_video_surface_getbits_ycbcr(orig->vdpau_accel_data.surface, format, this->vo_frame.base, this->vo_frame.pitches);
if (st != VDP_STATUS_OK)
printf("vo_vdpau: failed to get surface bits !! %s\n", vdp_get_error_string(st));

st = vdp_video_surface_putbits_ycbcr(this->vdpau_accel_data.surface, format, this->vo_frame.base, this->vo_frame.pitches);
if (st != VDP_STATUS_OK)
printf("vo_vdpau: failed to put surface bits !! %s\n", vdp_get_error_string(st));
And the output is like that when it crashes:
Code:
orig->vo_frame_width: 810, orig->vo_frame.height: 2849, format: 1, w: 810, h: 2852, ct: 0
Looks like the height was internally increased to a multiple of 4 to take account for interlacing and chroma resolution.

I'd like to avoid reading back the actually allocated size from the vdpau surface. I would be glad if you could handle that transparently.

Otherwise I'd have to round the frame size for example to macroblock units (i. e. 16x16 pixels) and crop away the excess.

Bye.

 08-31-09, 06:05 PM #4 Stephen Warren Moderator     Join Date: Aug 2005 Posts: 1,327 Re: VdpVideoSurfaceGet/PutBitsYCbCr() cause invalid reads/writes for frame size 1136x Yes, Putbits() is definitely using the rounded sizes, which is somewhat implicit. I'll file a bug and see if we can get that fixed. Thanks for the information.
 09-01-09, 03:02 PM #5 Stephen Warren Moderator     Join Date: Aug 2005 Posts: 1,327 Re: VdpVideoSurfaceGet/PutBitsYCbCr() cause invalid reads/writes for frame size 1136x I've looked into this some more, and it's not really possible to prevent, or hide, the size rounding. The reason is that a YV12 surface *has* to have a width that's a multiple of 2, due to chroma decimation (otherwise, the final chroma pixel doesn't have associated luma pixels to completely cover). A similar argument exists for height, except that additionally, this applies to each field, so the height must be a multiple of 4.
09-01-09, 04:21 PM   #6
rnissl
Registered User

Join Date: Jun 2005
Posts: 36
Re: VdpVideoSurfaceGet/PutBitsYCbCr() cause invalid reads/writes for frame size 1136x

Hi,

Quote:
 Originally Posted by Stephen Warren I've looked into this some more, and it's not really possible to prevent, or hide, the size rounding. The reason is that a YV12 surface *has* to have a width that's a multiple of 2, due to chroma decimation (otherwise, the final chroma pixel doesn't have associated luma pixels to completely cover). A similar argument exists for height, except that additionally, this applies to each field, so the height must be a multiple of 4.
Please document these constraints and I'll try to make xine-lib honor them.

Bye.

 09-01-09, 07:24 PM #7 Stephen Warren Moderator     Join Date: Aug 2005 Posts: 1,327 Re: VdpVideoSurfaceGet/PutBitsYCbCr() cause invalid reads/writes for frame size 1136x Sure. The following text will be included in vdpau.h in the next driver release: Code: /** * \brief Create a VdpVideoSurface. * \param[in] device The device that will contain the surface. * \param[in] chroma_type The chroma type of the new surface. * \param[in] width The width of the new surface. * \param[in] height The height of the new surface. * \param[out] surface The new surface's handle. * \return VdpStatus The completion status of the operation. * * The memory backing the surface may not be initialized during * creation. Applications are expected to initialize any region * that they use, via \ref VdpDecoderRender or \ref * VdpVideoSurfacePutBitsYCbCr. * * Note that certain widths/heights are impossible for specific values of * chroma_type. For example, the definition of VDP_CHROMA_TYPE_420 implies * that the width must be even, since each single chroma sample covers two * luma samples horizontally. A similar argument applies to surface heights, * although doubly so, since interlaced pictures must be supported; each * field's height must itself be a multiple of 2. Hence the overall surface's * height must be a multiple of 4. * * Similar rules apply to other chroma_type values. * * Implementations may also impose additional restrictions on the surface * sizes they support, potentially requiring additional rounding of actual * surface sizes. * * In most cases, this is not an issue, since: * - Video streams are encoded as an array of macro-blocks, which typically * have larger size alignment requirements than video surfaces do. * - APIs such as \ref VdpVideoMixerRender allow specification of a sub-region * of the surface to read, which allows the padding data to be clipped away. * * However, other APIs such as \ref VdpVideoSurfaceGetBitsYCbCr and * \ref VdpVideoSurfacePutBitsYCbCr do not allow a sub-region to be specified, * and always operate on surface size that was actually allocated, rather * than the surface size that was requested. In this case, applications need * to be aware of the actual surface size, in order to allocate appropriately * sized buffers for the get-/put-bits operations. * * For this reason, applications may need to call * \ref VdpVideoSurfaceGetParameters after creation, in order to retrieve the * actual surface size. */ typedef VdpStatus VdpVideoSurfaceCreate( VdpDevice device, VdpChromaType chroma_type, uint32_t width, uint32_t height, /* output parameters follow */ VdpVideoSurface * surface ); Hope this helps!