RESOLVED FIXED 116534
[GStreamer] Adjust internal size on http source element when receiving data if necessary
https://bugs.webkit.org/show_bug.cgi?id=116534
Summary [GStreamer] Adjust internal size on http source element when receiving data i...
Andre Moreira Magalhaes
Reported 2013-05-21 06:12:41 PDT
Follow up on bug #115354.
Attachments
Adjust internal size when receiving data if needed (2.44 KB, patch)
2013-05-21 06:15 PDT, Andre Moreira Magalhaes
no flags
Adjust internal size when receiving data if needed (2.44 KB, patch)
2013-05-21 06:19 PDT, Andre Moreira Magalhaes
pnormand: review+
cgarcia: commit-queue-
Adjust internal size when receiving data if needed (2.47 KB, patch)
2013-06-06 09:58 PDT, Andre Moreira Magalhaes
no flags
Adjust internal size when receiving data if needed (2.47 KB, patch)
2013-06-06 10:16 PDT, Andre Moreira Magalhaes
no flags
Andre Moreira Magalhaes
Comment 1 2013-05-21 06:15:44 PDT
Created attachment 202423 [details] Adjust internal size when receiving data if needed This patch replaces https://bugs.webkit.org/attachment.cgi?id=202290 from bug #115354
WebKit Commit Bot
Comment 2 2013-05-21 06:18:09 PDT
Attachment 202423 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp']" exit_code: 1 Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:917: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Andre Moreira Magalhaes
Comment 3 2013-05-21 06:19:05 PDT
Created attachment 202424 [details] Adjust internal size when receiving data if needed Updated Changelog to use proper bug number.
WebKit Commit Bot
Comment 4 2013-05-21 06:21:12 PDT
Attachment 202424 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp']" exit_code: 1 Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:917: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Carlos Garcia Campos
Comment 5 2013-06-06 06:07:23 PDT
Comment on attachment 202424 [details] Adjust internal size when receiving data if needed View in context: https://bugs.webkit.org/attachment.cgi?id=202424&action=review Please, fix coding style issues before landing. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1019 > + // priv->size == 0 if received length on didReceiveResponse < 0 Nit: According to the coding style comments should finish with a period. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1020 > + if (priv->size != 0 && priv->offset > priv->size) { This is confusing offset and size are both unsigned so if offset == 0 it will never be > size. This could probably just be if (priv->offset > priv->size) > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1021 > + GST_DEBUG_OBJECT(m_src, "Updating internal size from %ld to %ld", priv->size, priv->offset); size and offset are guint64, you should probably use G_GUINT64_FORMAT instead of %ld.
Andre Moreira Magalhaes
Comment 6 2013-06-06 09:58:43 PDT
Created attachment 203946 [details] Adjust internal size when receiving data if needed (In reply to comment #5) > (From update of attachment 202424 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=202424&action=review > > Please, fix coding style issues before landing. > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1019 > > + // priv->size == 0 if received length on didReceiveResponse < 0 > > Nit: According to the coding style comments should finish with a period. > Done > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1020 > > + if (priv->size != 0 && priv->offset > priv->size) { > > This is confusing offset and size are both unsigned so if offset == 0 it will never be > size. This could probably just be > > if (priv->offset > priv->size) > But IIRC, if priv->size == 0 (set in didReceiveResponse) we don't want/need to update the internal size as didReceiveResponse will disable seek, etc on the gst appsrc. No change here > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1021 > > + GST_DEBUG_OBJECT(m_src, "Updating internal size from %ld to %ld", priv->size, priv->offset); > > size and offset are guint64, you should probably use G_GUINT64_FORMAT instead of %ld. Done
WebKit Commit Bot
Comment 7 2013-06-06 10:00:06 PDT
Attachment 203946 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp']" exit_code: 1 Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1015: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Carlos Garcia Campos
Comment 8 2013-06-06 10:05:42 PDT
(In reply to comment #6) > Created an attachment (id=203946) [details] > Adjust internal size when receiving data if needed > > (In reply to comment #5) > > (From update of attachment 202424 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=202424&action=review > > > > Please, fix coding style issues before landing. > > > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1019 > > > + // priv->size == 0 if received length on didReceiveResponse < 0 > > > > Nit: According to the coding style comments should finish with a period. > > > Done > > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1020 > > > + if (priv->size != 0 && priv->offset > priv->size) { > > > > This is confusing offset and size are both unsigned so if offset == 0 it will never be > size. This could probably just be > > > > if (priv->offset > priv->size) > > > But IIRC, if priv->size == 0 (set in didReceiveResponse) we don't want/need to update the internal size as didReceiveResponse will disable seek, etc on the gst appsrc. No change here You are right, I read the comparison wrongly. But still, you should do: if (priv->size && priv->offset > priv->size) or if (priv->size > 0 && priv->offset > priv->size) to make the style bot happy.
Andre Moreira Magalhaes
Comment 9 2013-06-06 10:16:07 PDT
Created attachment 203947 [details] Adjust internal size when receiving data if needed (In reply to comment #8) > > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1020 > > > > + if (priv->size != 0 && priv->offset > priv->size) { > > > > > > This is confusing offset and size are both unsigned so if offset == 0 it will never be > size. This could probably just be > > > > > > if (priv->offset > priv->size) > > > > > But IIRC, if priv->size == 0 (set in didReceiveResponse) we don't want/need to update the internal size as didReceiveResponse will disable seek, etc on the gst appsrc. No change here > > You are right, I read the comparison wrongly. But still, you should do: > > if (priv->size && priv->offset > priv->size) > > or > > if (priv->size > 0 && priv->offset > priv->size) > > to make the style bot happy. hehe, done :).
WebKit Commit Bot
Comment 10 2013-06-19 06:23:22 PDT
Comment on attachment 203947 [details] Adjust internal size when receiving data if needed Clearing flags on attachment: 203947 Committed r151731: <http://trac.webkit.org/changeset/151731>
WebKit Commit Bot
Comment 11 2013-06-19 06:23:25 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.