Last Comment Bug 517363 - Preserve video poster aspect ratio when scaling
: Preserve video poster aspect ratio when scaling
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: mozilla15
Assigned To: Chris DeCairos (:cade)
:
Mentors:
: 712489 (view as bug list)
Depends on:
Blocks: 449156 1092378
  Show dependency treegraph
 
Reported: 2009-09-17 18:04 PDT by Chris Pearce (:cpearce)
Modified: 2014-10-31 15:00 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
minimized test case (50.51 KB, application/octet-stream)
2011-08-18 17:17 PDT, Kip
no flags Details
Ugly hack to get it working (3.98 KB, patch)
2012-02-08 22:06 PST, Chris DeCairos (:cade)
no flags Details | Diff | Review
Modified my original patch to use aMetrics (3.92 KB, patch)
2012-04-16 21:16 PDT, Chris DeCairos (:cade)
no flags Details | Diff | Review
Changes to patch, as per review (8.84 KB, patch)
2012-04-19 15:09 PDT, Chris DeCairos (:cade)
no flags Details | Diff | Review
Review fixes (9.50 KB, patch)
2012-05-09 23:02 PDT, Chris DeCairos (:cade)
roc: review+
Details | Diff | Review
Fixed up as per review, carrying forward from previous r+ (9.50 KB, patch)
2012-05-10 17:35 PDT, Chris DeCairos (:cade)
cade: review+
Details | Diff | Review

Description Chris Pearce (:cpearce) 2009-09-17 18:04:53 PDT
Currently if you have a video which is a different size than its width and height attributes specify, it's scaled WRT its aspect ratio to fit in its block. We don't scale video's poster WRT the aspect ratio though, we distort it to fill the width x height block. We should scale poster WRT aspect ratio. If we don't, we can't (for example) take a screen shot of a scaled video playing and used that as the poster attribute, as it will be distorted when it's scaled.

We should get the HTML spec changed as well for this.
Comment 1 Kip 2011-08-16 11:29:35 PDT
FWIW, the aspect ratio is preserved on Chrome, Safari (Windows and iOS), and Opera. IE9 is the only other browser I tested that behaves like FF.
Comment 2 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-08-16 13:30:38 PDT
Could we please get a minimized testcase attached to this bug?
Comment 3 Kip 2011-08-18 17:17:14 PDT
Created attachment 554264 [details]
minimized test case

When loading the page, you will see the poster image. It should be pillar-boxed, the same as the video when you hit play.  Instead, the image is stretched to fill the box. There should be a circle in the box, but it is stretched to an oval.
Comment 4 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-08-24 12:13:59 PDT
Bug confirmed on Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0

Can a developer look into what might be happening here?
Comment 5 Chris Pearce (:cpearce) 2012-01-12 20:10:54 PST
*** Bug 712489 has been marked as a duplicate of this bug. ***
Comment 6 Chris DeCairos (:cade) 2012-02-08 22:06:08 PST
Created attachment 595655 [details] [diff] [review]
Ugly hack to get it working

So This "fixes" the issue, but it's pretty ugly looking. It definitely needs to be cleaned up. Any suggestions/best practices on this?
Comment 7 Chris Pearce (:cpearce) 2012-02-10 01:55:25 PST
Comment on attachment 595655 [details] [diff] [review]
Ugly hack to get it working

gfx/layout stuff is more Roc's area than mine.

cadecairos: It would be helpful if you can also describe what the old code was doing wrong, and how this patch fixes the problem.
Comment 8 Chris DeCairos (:cade) 2012-02-10 08:11:22 PST
cpearce: Thanks for the tip!

So, before my patch, this bit of code was the problem(lines 338-341 in nsVideoFrame.cpp):

if (ShouldDisplayPoster()) {
        kidReflowState.SetComputedWidth(aReflowState.ComputedWidth());
        kidReflowState.SetComputedHeight(aReflowState.ComputedHeight());

It tells the frame that it should paint the image so it fills 100% of the frames width and height.

This bit here(lines 345-349):

ReflowChild(imageFrame, aPresContext, kidDesiredSize, kidReflowState,
                  mBorderPadding.left, mBorderPadding.top, 0, aStatus);
FinishReflowChild(imageFrame, aPresContext,
                        &kidReflowState, kidDesiredSize,
                        mBorderPadding.left, mBorderPadding.top, 0);

finishes the reflow. Important to note here are the mBorderPadding.left, and mBorderPadding.right parameters, which set the offsets of the image from the left and the top of the frame).

I'll go through what I've added:

+      PRUint32 posterHeight, posterWidth;
+      PRInt32 videoElemHeight, videoElemWidth;
+      gfxSize scaledRatio;

This sets up some variables I need. Annoyingly, the GetNaturalHeight method for images returns a unsigned int and the GetWidth method for the video element accepts an unsigned integer.  I use the gfxSize object to compute the size of the element according to its ratio.

+      nsCOMPtr<nsIDOMHTMLImageElement> posterImage = do_QueryInterface(mPosterImage);
+      NS_ENSURE_TRUE(posterImage, NS_ERROR_FAILURE);    
+      posterImage->GetNaturalHeight( &posterHeight );
+      posterImage->GetNaturalWidth( &posterWidth );

This bit just gets an interface to the anonymous image element, and gets it's natural height and width in pixels.

+      nsCOMPtr<nsIDOMHTMLVideoElement> videoDomElement = do_QueryInterface(mContent);
+      videoDomElement->GetWidth( &videoElemWidth );
+      videoDomElement->GetHeight( &videoElemHeight );

Similar to above, I get an interface to the video element and grab its height and width in pixels.

+      if ( !ShouldDisplayPoster() || !posterHeight || !posterWidth || !videoElemHeight || !videoElemWidth ) {
         kidReflowState.SetComputedWidth(0);
         kidReflowState.SetComputedHeight(0);

Check if anything is NULL or 0, if it is the computed width will be set to 0 (nothing to display)

+      } else {
+        gfxFloat scale = 
+          NS_MIN(static_cast<float>(videoElemWidth)/posterWidth, 
+                 static_cast<float>(videoElemHeight)/posterHeight);

I "borrowed" this bit from CorrectForAspectRatio. It calculates the scale on which to resize the height and width and grabs the lesser of the two. The reason I have to cast to float is so that I get a real number representing the actual scale.

+        scaledRatio = gfxSize(scale*posterWidth, 
+                              scale*posterHeight);

I'm not entirely convinced I need this, but right here I store the scaled pixel height and width in a gfxSize object.

+ kidReflowState.SetComputedWidth(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.width)));
+ kidReflowState.SetComputedHeight(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.height)));

Right here I take the calculated height and width and pass them into this handy method which converts the pixels to AppUnits. I use the calculated values to tell the frame what size the poster image should be.

ReflowChild(imageFrame, aPresContext, kidDesiredSize, kidReflowState,
+                 nsPresContext::CSSPixelsToAppUnits((videoElemWidth - static_cast<PRInt32>(scaledRatio.width))/2),
+                 nsPresContext::CSSPixelsToAppUnits((videoElemHeight - static_cast<PRInt32>(scaledRatio.height))/2), 0, aStatus);

I replaced the mBorderPadding values here with a calculation that determines the left and top position to draw the image at.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-12 17:31:16 PST
Comment on attachment 595655 [details] [diff] [review]
Ugly hack to get it working

Review of attachment 595655 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsVideoFrame.cpp
@@ +347,5 @@
> +      posterImage->GetNaturalWidth( &posterWidth );
> +
> +      nsCOMPtr<nsIDOMHTMLVideoElement> videoDomElement = do_QueryInterface(mContent);
> +      videoDomElement->GetWidth( &videoElemWidth );
> +      videoDomElement->GetHeight( &videoElemHeight );

This just gets the values from the DOM attributes (e.g., ignoring the effect of CSS rules). I don't understand why the layout of the poster image needs to depend on these attributes.
Comment 10 Chris Pearce (:cpearce) 2012-02-12 17:37:42 PST
I pushed the patch to TryServer, and got reftest failures:
https://tbpl.mozilla.org/?tree=Try&rev=52c122fd8e38

REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-7.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-10.html | image comparison (==)
Bug 721014 - Intermittent REFTEST TEST-UNEXPECTED-FAIL | ogg-video/poster-10.html | image comparison (==) REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-11.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-12.html | image comparison (==)
Bug 681170 - aspect-ratio-1a.xhtml, canvas-1b.xhtml, aspect-ratio-3a.xhtml, aspect-ratio-3b.xhtml, aspect-ratio-1b.xhtml, canvas-1a.xhtml, poster-12.html | assertion count 1 is more than expected 0 "ASSERTION: RECURSION_LEVEL(table_) > 0, file pldhash.cpp, line 707" REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-13.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-15.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-7.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-10.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-11.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-12.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-13.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-15.html | image comparison (==)
Comment 11 Chris DeCairos (:cade) 2012-02-12 18:50:28 PST
(In reply to Chris Pearce (:cpearce) from comment #10)
> I pushed the patch to TryServer, and got reftest failures:
> https://tbpl.mozilla.org/?tree=Try&rev=52c122fd8e38

Looks like these are happening because my patch breaks if a height and width on the element isn't specified, it won't draw the poster.

in reply to Comment 9, I don't think it should depend on those attributes. The reftest failures make that clear. How can I determine the actual height and width of the video element?
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-13 03:04:36 PST
When nsVideoFrame::Reflow is called, the computed width and height in aReflowState are the final width/height for the frame (which is why we set aMetrics.width/height to them at the beginning). So you just need to size and position the poster-image frame so it's letterboxed within (0,0,aMetrics.width,aMetrics.height).
Comment 13 Chris DeCairos (:cade) 2012-02-13 19:09:44 PST
If I have a video that's 300x400 and a poster image that's 150x200, and My video element has no explicit height and width, the element becomes the size of the small poster image and grows to the size of the video after playback begins. The reverse is true if the poster image is larger than the video is (shrinks after playback begins). Is this expected behavior? I would think the image should be scaled to the size of the video in the event there's no height and width.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-13 19:45:53 PST
(In reply to Chris DeCairos (:cadecairos) from comment #13)
> If I have a video that's 300x400 and a poster image that's 150x200, and My
> video element has no explicit height and width, the element becomes the size
> of the small poster image and grows to the size of the video after playback
> begins. The reverse is true if the poster image is larger than the video is
> (shrinks after playback begins). Is this expected behavior?

Apparently not:
http://www.whatwg.org/specs/web-apps/current-work/#dom-video-videowidth
"The intrinsic width of a video element's playback area is the intrinsic width of the video resource, if that is available; otherwise it is the intrinsic width of the poster frame, if that is available; otherwise it is 300 CSS pixels."

But I think that's a separate bug in nsVideoFrame::GetVideoIntrinsicSize. Don't worry about it in this bug. Feel free to file a bug on the sizing issue and fix that separately :-).
Comment 15 Chris DeCairos (:cade) 2012-02-13 19:55:25 PST
Great, I'll do just that. Although that problem may be why the reftests that cpearce reported were failing. I can't confirm that claim just yet though..
Comment 16 Chris DeCairos (:cade) 2012-04-16 21:16:44 PDT
Created attachment 615612 [details] [diff] [review]
Modified my original patch to use aMetrics

I've changed my patch to use aMetrics, which seems to work great. A couple reftests are broken, and I think the best way to "fix" them is to apply this patch on top of my work in Bug 726904, which also affects the same tests. I need to check if the reftests will pass with both the new behaviours present.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-17 00:47:34 PDT
Comment on attachment 615612 [details] [diff] [review]
Modified my original patch to use aMetrics

Review of attachment 615612 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsVideoFrame.cpp
@@ +285,5 @@
>                                         aMetrics.width,
>                                         aMetrics.height);
> +
> +      PRUint32 posterHeight, posterWidth;
> +      gfxSize scaledRatio;

Move the definition down below. Instead of saving scaledRatio for use later, save an nsSize which is the size after converting to appunits, so you don't have to keep repeating that.

@@ +292,5 @@
> +      NS_ENSURE_TRUE(posterImage, NS_ERROR_FAILURE);    
> +      posterImage->GetNaturalHeight( &posterHeight );
> +      posterImage->GetNaturalWidth( &posterWidth );
> +
> +      if ( !ShouldDisplayPoster() || !posterHeight || !posterWidth || !aMetrics.height || !aMetrics.width ) {

Line too long, break it. Also, lose spaces after ( and before ).

Don't need to check aMetrics/width/height here.

@@ +298,5 @@
>          kidReflowState.SetComputedHeight(0);
> +      } else {
> +        gfxFloat scale = 
> +          NS_MIN(static_cast<float>(aMetrics.width)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterWidth)), 
> +                 static_cast<float>(aMetrics.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterHeight)));

Casts to PRInt32 should not be needed.

@@ +300,5 @@
> +        gfxFloat scale = 
> +          NS_MIN(static_cast<float>(aMetrics.width)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterWidth)), 
> +                 static_cast<float>(aMetrics.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterHeight)));
> +        scaledRatio = gfxSize(scale*posterWidth, 
> +                              scale*posterHeight);

This is where you should declare scaledRatio. You can write it as "gfxSize scaledRatio(scale*posterWidth, scale*posterHeight)".

@@ +302,5 @@
> +                 static_cast<float>(aMetrics.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterHeight)));
> +        scaledRatio = gfxSize(scale*posterWidth, 
> +                              scale*posterHeight);
> +        kidReflowState.SetComputedWidth(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.width)));
> +        kidReflowState.SetComputedHeight(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.height)));

Don't need these PRInt32 casts.

@@ +311,4 @@
>        FinishReflowChild(imageFrame, aPresContext,
>                          &kidReflowState, kidDesiredSize,
> +                        (aMetrics.width - nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.width)))/2,
> +                        (aMetrics.height - nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.height)))/2, 0);

Compute the x/y once, store in an nsPoint, so you don't have to repeat these expressions.
Comment 18 Chris DeCairos (:cade) 2012-04-18 23:52:43 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)

> @@ +298,5 @@
> >          kidReflowState.SetComputedHeight(0);
> > +      } else {
> > +        gfxFloat scale = 
> > +          NS_MIN(static_cast<float>(aMetrics.width)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterWidth)), 
> > +                 static_cast<float>(aMetrics.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterHeight)));
> 
> Casts to PRInt32 should not be needed.
>
> @@ +302,5 @@
> > +                 static_cast<float>(aMetrics.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(posterHeight)));
> > +        scaledRatio = gfxSize(scale*posterWidth, 
> > +                              scale*posterHeight);
> > +        kidReflowState.SetComputedWidth(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.width)));
> > +        kidReflowState.SetComputedHeight(nsPresContext::CSSPixelsToAppUnits(static_cast<PRInt32>(scaledRatio.height)));
> 
> Don't need these PRInt32 casts.

I cast these variables here because CSSPixelsToAppUnits accepts either a 32 bit int or a float ( posterWidth/height are unsigned ints, and scaledRatio.height/width are doubles ) is there another way of doing this?
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-19 03:00:09 PDT
Cast to a float so the results doesn't have to be rounded to an integer.
Comment 20 Chris DeCairos (:cade) 2012-04-19 15:09:11 PDT
Created attachment 616766 [details] [diff] [review]
Changes to patch, as per review

This patch is based on the patch for Bug 726904.

I've made all the changes from roc's review, and a couple more based on my fixes.

I've also made the necessary changes to reftests that used to assume there was no poster frame scaling.

here's my tbpl push for this patch (includes 726904): https://tbpl.mozilla.org/?tree=Try&rev=2fde97b75aa7
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-19 20:11:20 PDT
Comment on attachment 616766 [details] [diff] [review]
Changes to patch, as per review

Review of attachment 616766 [details] [diff] [review]:
-----------------------------------------------------------------

What is poster-ref-blue125x100.html for? Nothing seems to use it.

::: layout/generic/nsVideoFrame.cpp
@@ +287,5 @@
>                                         aMetrics.height);
> +
> +      PRUint32 posterHeight, posterWidth;
> +      nsSize AUSize(0, 0),
> +             computedArea(aReflowState.ComputedWidth(), aReflowState.ComputedHeight());

AUSize is not a very meaningful name. Call it scaledPosterSize.

@@ +288,5 @@
> +
> +      PRUint32 posterHeight, posterWidth;
> +      nsSize AUSize(0, 0),
> +             computedArea(aReflowState.ComputedWidth(), aReflowState.ComputedHeight());
> +      nsPoint reflowSize(0, 0);

Call this posterTopLeft

@@ +293,5 @@
> +
> +      nsCOMPtr<nsIDOMHTMLImageElement> posterImage = do_QueryInterface(mPosterImage);
> +      NS_ENSURE_TRUE(posterImage, NS_ERROR_FAILURE);    
> +      posterImage->GetNaturalHeight( &posterHeight );
> +      posterImage->GetNaturalWidth( &posterWidth );

drop spaces after ( and before )

@@ +298,5 @@
> +
> +      if (ShouldDisplayPoster() && posterHeight && posterWidth) {
> +        gfxFloat scale = 
> +          NS_MIN(static_cast<float>(computedArea.width)/nsPresContext::CSSPixelsToAppUnits(static_cast<float>(posterWidth)), 
> +                 static_cast<float>(computedArea.height)/nsPresContext::CSSPixelsToAppUnits(static_cast<float>(posterHeight)));

You don't need to case posterWidth/posterHeight to float here.

::: layout/reftests/ogg-video/poster-ref-blue400x300.html
@@ +1,4 @@
>  <!DOCTYPE HTML>
>  <html>
>  <body style="background:white;">
> +<img src="blue250x200.png" style="position: absolute; width: 375px; height: 300px; left: 21px;">

left:21px is a bit magical. I guess it's (400 - (250/200)*300)/2 + 8, rounded (8 is the default margin-left). It would be better to use position:relative so that the results don't depend on the default margin. Also, it would be good to adjust sizes so that no rounding is required.
Comment 22 Chris DeCairos (:cade) 2012-05-09 18:54:28 PDT
unfortunately, nightly won't build without those casts to float. I've got a patch coming soon.
Comment 23 Chris DeCairos (:cade) 2012-05-09 19:31:06 PDT
poster-ref-blue125x100.html is used in the poster-10.html reftest in layout/reftests/ogg-video and layout/reftests/webm-video
Comment 24 Chris DeCairos (:cade) 2012-05-09 23:02:37 PDT
Created attachment 622629 [details] [diff] [review]
Review fixes

All changes were made, except those relating to my two above comments.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-10 00:42:17 PDT
Comment on attachment 622629 [details] [diff] [review]
Review fixes

Review of attachment 622629 [details] [diff] [review]:
-----------------------------------------------------------------

Attach a new patch with that fixed and we'll check it in! Thanks!

::: layout/generic/nsVideoFrame.cpp
@@ +287,5 @@
>                                         aMetrics.height);
> +
> +      PRUint32 posterHeight, posterWidth;
> +      nsSize scaledPosterSize(0, 0),
> +             computedArea(aReflowState.ComputedWidth(), aReflowState.ComputedHeight());

Declare computedArea with a separate nsSize declaration.
Comment 26 Chris DeCairos (:cade) 2012-05-10 17:35:22 PDT
Created attachment 622985 [details] [diff] [review]
Fixed up as per review, carrying forward from previous r+

All done :)

This patch needs to land on top of Bug 726904

How can we ensure this happens?
Comment 27 Chris Pearce (:cpearce) 2012-05-10 17:57:02 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=cfb9b2418c2b
Comment 29 Matt Brubeck (:mbrubeck) 2012-05-11 11:44:23 PDT
https://hg.mozilla.org/mozilla-central/rev/f968eb88b163

Note You need to log in before you can comment on or make changes to this bug.