Created attachment 312466 [details] [review]
fix assertion errors

Review of attachment 312466 [details] [review]:

::: gst/playback/gstdecodebin2.c
@@ +4449,3 @@
       gst_caps_unref (caps);
+      gst_structure_free (u);
+      return NULL;

Seems a bit extreme to directly return NULL here. What about returning incomplete information instead?

Also how can you even get unfixed caps here?

@@ +4468,3 @@
   s = gst_decode_chain_get_topology (dbin->decode_chain);
 
+  if (s) {

When does it return NULL? Should probably cause a GST_WARNING_OBJECT() somewhere.

(In reply to Sebastian Dröge (slomo) from comment #2)
> Review of attachment 312466 [details] [review] [review]:
> 
> ::: gst/playback/gstdecodebin2.c
> @@ +4449,3 @@
>        gst_caps_unref (caps);
> +      gst_structure_free (u);
> +      return NULL;
> 
> Seems a bit extreme to directly return NULL here. What about returning
> incomplete information instead?
> 
> Also how can you even get unfixed caps here?
> 
I am not sure if this scenario is possible.
While debugging another issue, just saw this piece of code,
where when the caps is unfixed, then 
      gst_caps_unref (caps);
      caps = NULL;
is being done, and the same will be passed on to
  gst_structure_id_set (u, topology_caps, GST_TYPE_CAPS, caps, NULL);
Passing NULL to this will cause assertion error.

By incomplete information you mean all other information without caps right?
But not sure if that makes much sense without caps :)


> @@ +4468,3 @@
>    s = gst_decode_chain_get_topology (dbin->decode_chain);
>  
> +  if (s) {
> 
> When does it return NULL? Should probably cause a GST_WARNING_OBJECT()
> somewhere.
Again this is a highly unlikely scenario :)
In the function gst_decode_chain_get_topology

  if (G_UNLIKELY ((chain->endpad || chain->deadend)
          && (chain->endcaps == NULL))) {
    GST_WARNING ("End chain without valid caps !");
    return NULL;
  }
So here it might return NULL. Hence that check was needed

How do you reproduce this issue? What are the unfixed caps you get there? That seems like the main bug here.

:)
I did not reproduce this issue...

I have another issue where gst-validate complains about caps mismatch similar to bug id 752690..

So i was just going through the decodebin code and found this code.. this is just cosmetic changes..

While it is somewhat cosmetic, we should find the reason why it gets into that situation. It shouldn't happen :)

Created attachment 316073 [details] [review]
fix get_topology logic to remove possible assertion errors

gst_decode_chain_get_topology will be called when the decode chain is being exposed. At this time the chain caps will be fixed. So there is no need to check if the caps is fixed or not.
And removing duplicate gst_pad_get_current_caps function, as the same will be already done in get_pad_caps. And if get_pad_caps returns NULL(which is unlikely), just return the incomplete caps. Because passing NULL caps to 
   gst_structure_id_set (u, topology_caps, GST_TYPE_CAPS, caps, NULL);
will cause assertion errors.
And in case endchain doesnt have any valid caps(which is again unlikely), then get_topology returns NULL, which will cause assertion errors when passed to gst_message_new_element. Hence adding a precautionary check.

ping :)

Review of attachment 316073 [details] [review]:

When exactly will this return NULL caps? The only case I currently think of is when decodebin is being shut down while this code runs. Anything else? Like missing decoders?

Makes sense otherwise.

nope.. I dont think if there are missing decoders, it will even come till posting the topology message.

So what exactly is the reason for the NULL caps, when does it happen? :) I'm worried that there might be another bug that is only hidden by your patch.

Ping?

as mentioned in previous comments, i don't see any reason why the caps will be NULL, other than maybe when decodebin shuts down when this code is running.
It was already added just as a precautionary measure. So just adding extra precautionary checks. 


The only change needed here is replacing the below block of code

  /* Caps that resulted in this chain */
  caps = gst_pad_get_current_caps (chain->pad);
  if (!caps) {
    caps = get_pad_caps (chain->pad);
    if (G_UNLIKELY (!gst_caps_is_fixed (caps))) {
      GST_ERROR_OBJECT (chain->pad,
          "Couldn't get fixed caps, got %" GST_PTR_FORMAT, caps);
      gst_caps_unref (caps);
      caps = NULL;
    }
  }
...

with
caps = get_pad_caps (chain->pad);

since get_pad_caps already handles gst_pad_get_current_caps, so it will become duplicated calls


Rest all is not needed and highly impossible scenarios. Hence the addition of G_UNLIKELY :)

Actually it is not even a major issue, so i don't mind to close this issue as invalid.

commit 5d78aab81081d157826e0228dc310b4fae9eb8e0
Author: Vineeth T M <vineeth.tm@samsung.com>
Date:   Mon Nov 23 15:06:02 2015 +0900

    decodebin: return incomplete topology if decode chains' cap could not be obtained
    
    When getting caps of the decode chain, in get_topology, the caps are being
    checked if fixed or not. But get_topology will be called when the decode is
    chain is being exposed and hence it will always be fixed. Hence removing the
    check for fixed caps. Removing gst_pad_get_current_caps for the chain->pad, as
    get_pad_caps will again call the same api.
    
    And get_topology can return NULL value if currently shutting down the
    pipeline, which on being passed to create message will result in assertion
    error. Check if topology is valid before using it
    
    https://bugzilla.gnome.org/show_bug.cgi?id=755918

I see, that makes sense then :)