GNOME Bugzilla – Bug 753011
queue2: can not update upstream_size with valid data
Last modified: 2015-08-16 13:38:06 UTC
queue->upstream_size is set to -1 at first then it is never updated. Because the queue->upstream_size is unsigned value so it will be 18446744073709551615. so gst_queue2_update_upstream_size() is never called again. I've tested with below command. gst-launch-1.0 souphttpsrc location="http://content.bitsontherun.com/videos/ntPYsD4L-1ahmry41.mp4" ! queue2 ring-buffer-max-size=10485760 ! decodebin ! autovideosink
Created attachment 308375 [details] [review] queue2: update upstream size with valid size queue->upstream_size is set to -1 at first then it is never updated. Because the queue->upstream_size is unsigned value so it will be 18446744073709551615. so gst_queue2_update_upstream_size() is never called again. To make it update, upstream_size should be 0 like default value. I added patch here. And, the souphttpsrc has an issue too. If the Range value over the content_size, some kinds of server response is 200 even if it support seeking. so I am making another patch for it. Do I need to make new bug item for this?
Created attachment 308377 [details] [review] queue2: update upstream size with valid size attach patch again.
>> And, the souphttpsrc has an issue too. >> If the Range value over the content_size, some kinds of server response is 200 even if it support seeking. so I am making another patch for it. >> Do I need to make new bug item for this? I registered new bug item and patch. https://bugzilla.gnome.org/show_bug.cgi?id=753178
gst_pad_peer_query_duration () updates the value in upstream_size. Are you sure after that function runs and returns True, the value of upstream_size can be negative in the conditional block?
the upstream_size from gst_pad_peer_query_duration() can be negative. in gst_base_src_default_query(), duration value can be -1 with true return. and when I tested the above command line I got negative one. to update the queue->upstream_size in gst_queue2_get_range(), the value should be default not 18446744073709551615.
True! Interesting. The first two times gst_queue2_update_upstream_size() runs in your example pipeline the upstream_size is set to -1. I can reproduce the bug and confirm the patch fixes it. Not confident enough to approve and merge this patch. Eunhae, I think that if you are going to add that (>= 0) check you should write a comment explaining why it is needed just above it.
Review of attachment 308377 [details] [review]: The file mode was mistakenly updated in this patch. ::: plugins/elements/gstqueue2.c @@ +3178,3 @@ GST_INFO_OBJECT (queue, "upstream size: %" G_GINT64_FORMAT, upstream_size); + if (upstream_size >= 0) + queue->upstream_size = upstream_size; How about adding an else here to catch if it is < 0 and in this case reset the upstream_size back to 0? I can't think of any real life use case where that might happen but seems safer than keeping upstream_size with an obsolete value.
Created attachment 308718 [details] [review] queue2: not to update upstream size with negative value Thank you for your review. I've uploaded new patch. 1) add comment 2) add else phrase to set queue->upstream_size with default value.
Review of attachment 308718 [details] [review]: Did a slight modification to the comment in the code and pushed. Thanks for the patch. commit ed7e0e744fa3190224a0e9525d80dd33e7d9283e Author: Eunhae Choi <eunhae1.choi@samsung.com> Date: Tue Aug 4 13:45:09 2015 +0900 queue2: not update upstream size with negative value upstream_size can be negative but queue->upstream_size is unsigned type. to get a chance to update queue->upstream_size in gst_queue2_get_range() it should keep the default value. https://bugzilla.gnome.org/show_bug.cgi?id=753011