Can you check the other audio/video parsers too for more of the similar problem?

(In reply to Sebastian Dröge (slomo) from comment #1)
> Can you check the other audio/video parsers too for more of the similar
> problem?
Yes, I did take a look. None of the audio or video parsers looked to have a similar problem.

However, some of the other plugins had things that I thought looked sketchy. Using gst_pad_has_current_caps() to decide whether to call gst_pad_get_current_caps e.g. from gst_flac_enc_sink_query


      if (gst_pad_has_current_caps (pad)) {
        acceptable = gst_pad_get_current_caps (pad);
      } else {
        acceptable = gst_pad_get_pad_template_caps (pad);
      }

This would seem better:

      acceptable = gst_pad_get_current_caps (pad);
      if(acceptable == NULL) {
        acceptable = gst_pad_get_pad_template_caps (pad);
      }

Anywhere that calls gst_pad_get_current_caps based on the result of gst_pad_has_current_caps looks to me to be problematic. These are the ones I found:

gst_flac_enc_getcaps
gst_flac_enc_sink_query
gst_shape_wipe_video_sink_getcaps
gst_shape_wipe_mask_sink_getcaps
gst_shape_wipe_src_getcaps
gst_flv_mux_create_metadata
gst_image_freeze_sink_getcaps
gst_aspect_ratio_crop_set_property
gst_rtp_h264_set_src_caps
gst_disparity_handle_query
gst_rtp_h265_set_src_caps

The other places that looked problematic were in demuxes, though it may be that these are ok e.g. call to gst_pad_get_current_caps in gst_deinterlace_output_frame. I guess all calls to gst_pad_get_current_caps checking for NULL is the only way to be sure.

Agreed

Created attachment 317495 [details] [review]
Patch for plugins-bad gst_pad_has_current_caps abuse

Created attachment 317496 [details] [review]
Patch for plugins-good gst_pad_has_current_caps abuse

Thanks, did you also check gst-plugins-ugly, gst-plugins-base, gst-libav and gstreamer core?

(In reply to Sebastian Dröge (slomo) from comment #6)
> Thanks, did you also check gst-plugins-ugly, gst-plugins-base, gst-libav and
> gstreamer core?
I checked gstreamer-core and it looked ok, but nothing else. I'll be working through them over the next few days.

Thanks, looks all good so far (but please run gst-indent on your future changes, on the source files only, not headers).

I expect some more of these in gst-plugins-base, including some places in e.g. the audio/video codec base classes.

Where exactly are the caps removed from the pad btw? I wonder if that shouldn't take the stream lock and as not just drop the caps while data processing happens.

> Where exactly are the caps removed from the pad btw?
For the assert in comment 1 I think it's happening as I described in bug 759503 comment 1 - pre_activate inside gst_pad_activate_mode. However, that may not be correct. I had originally imagined that a larger lock (stream lock) would be in use but I'm not overly clear on the gstreamer locking strategy.

post_activate() will remove all the events, and as such the caps. And that one is taking the stream lock.

Problem here is that this happens on the srcpad... and the srcpad's stream lock is not taken during data processing in all these elements. Only the sinkpad's stream lock. So I guess for all these patches, only the ones related to srcpads are valid.

Anyway, I'll merge all this when you or someone else went through the other modules. For the srcpads we should do checks like this, it also seems a bit silly to split the checks up into two parts (if (has) get) instead of just getting and checking for NULL.

We should probably also add a comment to the docs of gst_pad_has_current_caps() regarding that.