Go Back   nV News Forums > Linux Support Forums > NVIDIA Linux

Newegg Daily Deals

Reply
 
Thread Tools
Old 08-29-09, 05:50 PM   #1
rnissl
Registered User
 
Join Date: Jun 2005
Posts: 36
Default 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
File Type: txt valgrind.txt (28.1 KB, 86 views)
rnissl is offline   Reply With Quote
Old 08-31-09, 12:28 PM   #2
Stephen Warren
Moderator
 
Stephen Warren's Avatar
 
Join Date: Aug 2005
Posts: 1,327
Default 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.
Stephen Warren is offline   Reply With Quote
Old 08-31-09, 03:13 PM   #3
rnissl
Registered User
 
Join Date: Jun 2005
Posts: 36
Default Re: VdpVideoSurfaceGet/PutBitsYCbCr() cause invalid reads/writes for frame size 1136x

Hi,

Quote:
Originally Posted by Stephen Warren View Post
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.
rnissl is offline   Reply With Quote
Old 08-31-09, 05:05 PM   #4
Stephen Warren
Moderator
 
Stephen Warren's Avatar
 
Join Date: Aug 2005
Posts: 1,327
Default 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.
Stephen Warren is offline   Reply With Quote
Old 09-01-09, 02:02 PM   #5
Stephen Warren
Moderator
 
Stephen Warren's Avatar
 
Join Date: Aug 2005
Posts: 1,327
Default 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.
Stephen Warren is offline   Reply With Quote
Old 09-01-09, 03:21 PM   #6
rnissl
Registered User
 
Join Date: Jun 2005
Posts: 36
Default Re: VdpVideoSurfaceGet/PutBitsYCbCr() cause invalid reads/writes for frame size 1136x

Hi,

Quote:
Originally Posted by Stephen Warren View Post
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.
rnissl is offline   Reply With Quote
Old 09-01-09, 06:24 PM   #7
Stephen Warren
Moderator
 
Stephen Warren's Avatar
 
Join Date: Aug 2005
Posts: 1,327
Default 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!
Stephen Warren is offline   Reply With Quote
Reply


Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Forum Jump


All times are GMT -5. The time now is 05:19 AM.


Powered by vBulletin® Version 3.7.1
Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Copyright 1998 - 2014, nV News.