Bug 116534 – [GStreamer] Adjust internal size on http source element when receiving data if necessary

• Component: Platform
• Status: RESOLVED FIXED

[Andre Moreira Magalhaes]

Follow up on bug #115354.

[Andre Moreira Magalhaes]

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]

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]

Created attachment 202424 [details]
Adjust internal size when receiving data if needed 

Updated Changelog to use proper bug number.

[WebKit Commit Bot]

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 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]

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]

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]

(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]

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 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]

All reviewed patches have been landed.  Closing bug.