GNOME Bugzilla – Bug 766184
vaapivideopool: add video meta if required
Last modified: 2016-06-15 18:45:46 UTC
Created attachment 327529 [details] Test videos I've been running some tests with gstreamer 1.8.1 and gstreamer-vaapi 1.8.1 using attached video and these commands : gst-launch-1.0 filesrc location=~/Videos/perform-this-way-5-secs.flv ! decodebin ! videoconvert ! videoscale ! video/x-raw, width=1920, height=1080 ! autovideosink gst-launch-1.0 filesrc location=~/Videos/perform-this-way-5-secs.flv ! decodebin ! tee name=t t.! queue ! videoconvert ! videoscale ! video/x-raw, width=1920, height=1080 ! autovideosink Once we use tee element we will have distortion in output, I've tested this with 1080P mpeg2 videos and again if we use tee element we will have distortion in output while 1080P H264 playback is fine. this will not happen if we use software decoder but I'm not sure VAAPI is causing this or tee element
Hi, Can you tell us the output of the vainfo command in your system? In that way we can know which backend/chipset you are using.
(In reply to Víctor Manuel Jáquez Leal from comment #1) > Hi, > > Can you tell us the output of the vainfo command in your system? > In that way we can know which backend/chipset you are using. this is the vainfo output : libva info: VA-API version 0.39.0 libva info: va_getDriverName() returns 0 libva info: Trying to open /usr/lib64/dri/i965_drv_video.so libva info: Found init function __vaDriverInit_0_39 libva info: va_openDriver() returns 0 vainfo: VA-API version: 0.39 (libva 1.7.1.pre1) vainfo: Driver version: Intel i965 driver for Intel(R) Skylake - 1.7.0 vainfo: Supported profile and entrypoints VAProfileMPEG2Simple : VAEntrypointVLD VAProfileMPEG2Simple : VAEntrypointEncSlice VAProfileMPEG2Main : VAEntrypointVLD VAProfileMPEG2Main : VAEntrypointEncSlice VAProfileH264ConstrainedBaseline: VAEntrypointVLD VAProfileH264ConstrainedBaseline: VAEntrypointEncSlice VAProfileH264Main : VAEntrypointVLD VAProfileH264Main : VAEntrypointEncSlice VAProfileH264High : VAEntrypointVLD VAProfileH264High : VAEntrypointEncSlice VAProfileH264MultiviewHigh : VAEntrypointVLD VAProfileH264MultiviewHigh : VAEntrypointEncSlice VAProfileH264StereoHigh : VAEntrypointVLD VAProfileH264StereoHigh : VAEntrypointEncSlice VAProfileVC1Simple : VAEntrypointVLD VAProfileVC1Main : VAEntrypointVLD VAProfileVC1Advanced : VAEntrypointVLD VAProfileNone : VAEntrypointVideoProc VAProfileJPEGBaseline : VAEntrypointVLD VAProfileJPEGBaseline : VAEntrypointEncPicture VAProfileVP8Version0_3 : VAEntrypointVLD VAProfileVP8Version0_3 : VAEntrypointEncSlice VAProfileHEVCMain : VAEntrypointVLD VAProfileHEVCMain : VAEntrypointEncSlice That been said, if we don't put tee element after decodebin we still use vaapi and the output will not be distorted so I guess the file format is supported. I get same result with a Haswell machine.
This happens because tee doesn't proxy allocation queries (by design, I think) and there are some problems with the correctness of the outputs of the vaapi elements when the downstream elements don't take the video meta. I'll work on a patch for this.
(In reply to Scott D Phillips from comment #3) > This happens because tee doesn't proxy allocation queries (by design, I > think) and there are some problems with the correctness of the outputs of > the vaapi elements when the downstream elements don't take the video meta. > I'll work on a patch for this. Tee indeed don't proxy allocation, it's not by design, but because we don't yet know how to aggregated the queries. We should probably try and aggregate metas like VideoMeta, as it will cause copies in correctly implemented element.
Created attachment 329718 [details] [review] [PATCH 1/3] plugins: retry pool config
Created attachment 329719 [details] [review] [PATCH 2/3] vaapivideobufferpool: add video meta to config when needed
Review of attachment 329718 [details] [review]: Looks good to me. Victor, is there any extra configuration that the pool should not have changed and that should be checked ? gst_buffer_pool_config_validate_params() checks the most common one, but for stack like VAAPI I would not be surprised if there is extra restriction.
Created attachment 329720 [details] [review] [PATCH 3/3] plugins: Add caps parameter to base decide_allocation These patches add behavior that is like v4l2, which decides that a video meta is required when setting the buffer pool config. Víctor, I had to plumb the real caps through to the base decide_allocation because it only had the allocation_caps through the query. Look like the right way to handle that?
Review of attachment 329719 [details] [review]: No, not this way. The video meta being an extension, element that uses this pool without video meta will now do the wrong thing. In correct implementation of decide allocation, the meta should have been enabled if possible, and the set_config should just keep failing if not supported.
(In reply to Nicolas Dufresne (stormer) from comment #9) > Review of attachment 329719 [details] [review] [review]: > > No, not this way. The video meta being an extension, element that uses this > pool without video meta will now do the wrong thing. In correct > implementation of decide allocation, the meta should have been enabled if > possible, and the set_config should just keep failing if not supported. If I'm understanding, this patch has the right behavior for set_config but you're saying decide_allocation should then notice that the video meta option got added, which isn't supported in its final query and then fail, is that right? The buffer pool code in patch 2 is similar to how v4l2's pool set_config works if I'm reading the code there right, specifically: https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=ba32cf10f3e8898f3700979796f8886852dd0b7f Or do I have that wrong? Maybe it would be better to realize that the meta will be required earlier and put it in the caps instead of handling it with the allocation query?
(In reply to Scott D Phillips from comment #10) > (In reply to Nicolas Dufresne (stormer) from comment #9) > > Review of attachment 329719 [details] [review] [review] [review]: > > > > No, not this way. The video meta being an extension, element that uses this > > pool without video meta will now do the wrong thing. In correct > > implementation of decide allocation, the meta should have been enabled if > > possible, and the set_config should just keep failing if not supported. > > If I'm understanding, this patch has the right behavior for set_config but > you're saying decide_allocation should then notice that the video meta > option got added, which isn't supported in its final query and then fail, is > that right? The buffer pool code in patch 2 is similar to how v4l2's pool > set_config works if I'm reading the code there right, specifically: > > https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/ > ?id=ba32cf10f3e8898f3700979796f8886852dd0b7f > > Or do I have that wrong? Maybe it would be better to realize that the meta > will be required earlier and put it in the caps instead of handling it with > the allocation query? It's a good point, still I'm wondering if this was right or not. In the end the negotiation works if you check that the returned config is compatible with what you support. With options like VideoMeta (which you must support if enabled) it's tricky, because if you don't support video meta, you can't check if it's present or not. My patch on v4l2 is probably breaking the case where an upstream element receive that pool, but don't support video meta.
I'd say let's assume this is correct for now, I don't think any element without video meta support where ever updated to "recheck/negotiate" the configuration. In the long run, we'll need to think about adding more helpers around configuration, like having a template that describe what one supports, rather then checking for fixed parameters.
Review of attachment 329718 [details] [review]: I guess it is OK to have this check.
Review of attachment 329719 [details] [review]: ::: gst/vaapi/gstvaapivideobufferpool.c @@ +184,3 @@ + GST_VIDEO_INFO_PLANE_OFFSET (&new_vip, i) != + GST_VIDEO_INFO_PLANE_OFFSET (alloc_vip, i)) + need_video_meta = TRUE; what about a break statement after true? @@ +185,3 @@ + GST_VIDEO_INFO_PLANE_OFFSET (alloc_vip, i)) + need_video_meta = TRUE; + } I feel that it would be better to move this check after the allocator is set, and to use priv->alloc_vip and priv->video_info
(In reply to Scott D Phillips from comment #8) > Created attachment 329720 [details] [review] [review] > [PATCH 3/3] plugins: Add caps parameter to base decide_allocation > > These patches add behavior that is like v4l2, which decides that a video > meta is required when setting the buffer pool config. > > Víctor, I had to plumb the real caps through to the base decide_allocation > because it only had the allocation_caps through the query. Look like the > right way to handle that? It doesn't seem right. We should use what is in the query, otherwise we would be breaking what we did for bug 753914 and bug 764316. Perhaps we should decide to set the allocation_caps in the output state depending on the used buffer pool, or something.
Created attachment 329814 [details] [review] [PATCH v2] vaapivideobufferpool: add video meta to config when needed (In reply to Víctor Manuel Jáquez Leal from comment #14) > Review of attachment 329719 [details] [review] [review]: > ... > what about a break statement after true? done > ... > I feel that it would be better to move this check after the allocator is > set, and to use priv->alloc_vip and priv->video_info done
Pushed. But we need to backport it to branch 1.8, where the vaapi bufferpool is quite a different beast now. Scott, can you do it?
(In reply to Víctor Manuel Jáquez Leal from comment #17) > Pushed. But we need to backport it to branch 1.8, where the vaapi bufferpool > is quite a different beast now. > > Scott, can you do it? Sure, I can do it.
Created attachment 329868 [details] [review] [PATCH 1.8 1/2] plugins: retry pool config
Created attachment 329869 [details] [review] [PATCH 1.8 2/2] vaapivideobufferpool: add video meta to config when needed Backported patches to 1.8