GNOME Bugzilla – Bug 684341
[codecparsers vc1] many vc1 stream give corruption image when gst-vaapi is used
Last modified: 2015-07-27 22:54:37 UTC
when gstreamer-vaapi is used to playback some vc1 stream, many streams result in corruption images. after some debug, it shows the root reason is: ttmbf is not set when vstransform is not set details see below: spec 8.3.6.2.1 Transform Type Selection If variable-sized transform coding is not enabled, then the 8x8 transform shall be used for all blocks. I have to say the above spec isn't expressed clearly: ttfrm is mentioned clearly, but not ttmbf. anyway, since ttfrm is only specified when ttmbs is set, we can deduce that ttmbf should set to 1. it is also consistent with ffmpeg vc1 decoder and hw behaviour (Intel GEN gfx)
Created attachment 224684 [details] [review] patch for fix patch for fix when vstransform is not set set ttmbf to 1 set ttfrm to 0 (8x8 block)
I believe codecparsers used to set defaults only for syntax elements when a standard explicitly mentions so. In this case, implied values are to be handled by either the codec layer, or the driver. Some hardware will already handle implied values properly. Reading down the Intel Gen specs, setting TTFRM to 0 and TTMBF to 1 is an explicit requirement when VSTRANSFORM is 0 and long mode is used. i.e. it should be programmed this way in the driver.
you explanation sounds good. however, from data flow, it's better to translate implicit statement to clear value as early as possible; it improves the robustness of the pipeline. otherwise we have to depend on other layer to translate it -- there are various of implementation for them. ffmpeg handles it in frame header parser. sometimes (in libva) we compares headers sent from ffmpeg and that from gstv-aapi for debug. if we rely on driver to make the fix, the asymmetry will lead to some confusion.
(In reply to comment #2) > Some hardware will already handle implied values properly. Reading down the > Intel Gen specs, setting TTFRM to 0 and TTMBF to 1 is an explicit requirement > when VSTRANSFORM is 0 and long mode is used. i.e. it should be programmed this > way in the driver. my understanding is: hw spec means that upper layer should meet the requirement, not specifically to be done in driver. it could also happen in: parser/codec/libva/driver. I'd prefer parse does it, since it's parser's role and responsibility to parse frame parameters.
What's up with this? Is this still relevant after all the recent other vc-1 changes? I have not looked at the actual issue at hand, but it sounds to me like you both agree that the patch is correct in terms of what values/state is implied, and you disagree over whether these should be set or not (rather than let anyone reading the fields deal with the implied values themselves)? It seems to me that if certain values are implied, we may just as well set them, otherwise they'll just be set to 0 or a random value (i.e. likely a wrong value), depending on whether the struct was cleared or not, and it should not do any harm to set them, should it? Only problem I can see is with being inconsistent in this respect and not doing it everywhere for all implied values.
Closing this bug report as no further information has been provided. Please feel free to reopen this bug report if you can provide the information that was asked for in a previous comment. Thanks!