Created attachment 108398 [details] [review]
Suggested patch

The displayed speeds should be even, not odd. They *were* even in Brasero 0.6.1!

Additional info on optical unit models: in my case it happens *always*, with several distros (at least Fedora, Ubuntu, Parsix), and on both a laptop and a PC!

Both optical units are by LG:
-- laptop unit: HL-DT-ST DVDRAM GSA-T20N. Capabilities: CD-R: 24x; DVD-R/DVD+R:
6x ZCLV.
-- PC unit: GCE-8526B. Capabilities: CD-R: 52x.

These models are reported in dmesg anyway. I can't run sginfo, but these units
are *very* popular. 

I can't believe that *only* the LG units report speeds in KiB or in KB that get
computed in incorrect Nx speed values in Brasero >= 0.6.90!

If I am wrong and the bug is much more complex (the initial Ubuntu reporter had weird speeds with DVDs, not with CDs, and the Fedora corresponding bug already has a maintainer's comment saying the bug can't be reproduced at him), then maybe it has something to do with the way the code was rewritten for the 0.7 branch, possibly with the definitions in src/burn-medium.h:

/* rates are in Kio/sec */
#define CD_RATE 153600
#define DVD_RATE 1385000

#define BRASERO_SPEED_TO_RATE_CD(speed)		(guint) ((speed) * CD_RATE)
#define BRASERO_SPEED_TO_RATE_DVD(speed)	(guint) ((speed) * DVD_RATE)
#define BRASERO_RATE_TO_SPEED_CD(rate)		(guint) ((rate) / CD_RATE)
#define BRASERO_RATE_TO_SPEED_DVD(rate)		(guint) ((rate) / DVD_RATE)

I don't know the exact 1x CD and DVD speeds in bytes (and KiB, not KB), I don't know what are the drives reporting (KiB or KB?), and I don't know whether we should truncate, floor or ceil to determine the correctly rounded Nx speed!

The base 1X speeds are precisely the defines above (CD_RATE and DVD_RATE). Whether to divide by the CD_RATE or DVD_RATE depends on the type of medium you inserted in the CD. Based on the modes data you reported on the Fedora bug :

>> MM capabilities and mechanical status (obsolete), page_control: current
 00     2a 3e 3f 37 f1 73 29 23  10 8a 01 00 08 00 10 8a                    
 10     00 00 10 8a 10 8a 00 01  00 00 00 00 10 8a 00 04 <- 4 speeds                    
 20     00 00 10 8a 00 00 0b 06  00 00 06 e4 00 00 02 c2                    
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This reports 4 available writeable speeds :

 - 28.2 x
 - 18.4 x
 - 11.5 x
  - 4.6 x

(assuming you had either a CD inserted or no medium at all).

Anyways, the bug is triggered by the simplistic heuristic brasero uses to approximate the supported burning speeds. Brasero doesn't actually use the reported list above (why???), it only uses the maximum value, then uses a simplistic algorithm to decrease that value 2 rates at a time.

I can actually reproduce this bug, but only with DVD burning speeds. My DVD burner reports supported write speeds of 5540 (exactly 4x) and 2770 (exactly 2x), yet the drive properties gives me choices of "Max speed", "3x" and "1x", the last 2 are not supported speeds...

The simple algo often works on CDs because CD usually report speeds slightly lower than advertized, i.e. instead of 12x, it'll report, say, 11.5x. You remove a rate out of that than truncate/divide by the CD rate and you get 10, then 8, 6, 4, 2...

This really ought to be fixed to use the burning speeds reported by GET PERFORMANCE...

Indeed, it's known that Brasero only steps down from the maximum speed and doesn't use the actual values...

OK, brilliant your arguments, thank you, this makes sense.

Supposing that 99% of the drives really report CD speeds lower than the actual values, e.g. 11.5x instead of 12x, this explains why you should decrement it to get an even value: 11.4x -> 11x -> 10x.

However, this doesn't explain *why* the previous (stable) version, 0.6.1, which had a completely different mechanism, gives me perfectly even CD burning speeds!!!

Here's the relevant Brasero 0.6.1 speed code:

/*=====================================================================*/
In /usr/include/libnautilus-burn/nautilus-burn-drive.h:98:
#define NAUTILUS_BURN_DRIVE_CD_SPEED(speed) (floorf (((speed) * 1024 / 153600.0) * 10) / 10.0)
#define NAUTILUS_BURN_DRIVE_DVD_SPEED(speed) (floorf (((speed)  * 1024 / 1385000.0) * 10) / 10.0)

In brasero-0.6.1/src/recorder-selection.c:1124:

        /* Speed combo */
        media = NCB_MEDIA_GET_STATUS (drive);
        max_rate = NCB_MEDIA_GET_MAX_WRITE_SPEED (drive);

        combo = gtk_combo_box_new_text ();
        if (media & BRASERO_MEDIUM_DVD)
                max_speed = NAUTILUS_BURN_DRIVE_DVD_SPEED (max_rate);
        else
                max_speed = NAUTILUS_BURN_DRIVE_CD_SPEED (max_rate);

        gtk_combo_box_prepend_text (GTK_COMBO_BOX (combo), _("Max speed"));
        for (i = max_speed; i > 0; i -= 2) {
                text = g_strdup_printf ("%i x (%s)",
                                        i,
                                        (media & BRASERO_MEDIUM_DVD) ? _("DVD"):_("CD"));
                gtk_combo_box_append_text (GTK_COMBO_BOX (combo), text);
                g_free (text);
        }
/*=====================================================================*/

Now the speed code for Brasero 0.7.0 and 0.7.1:

/*=====================================================================*/
In brasero-0.7.0/src/burn-medium.h:34:
In brasero-0.7.1/src/burn-medium.h:34:
/* rates are in Kio/sec */
#define CD_RATE 153600
#define DVD_RATE 1385000

#define BRASERO_SPEED_TO_RATE_CD(speed)         (guint) ((speed) * CD_RATE)
#define BRASERO_SPEED_TO_RATE_DVD(speed)        (guint) ((speed) * DVD_RATE)
#define BRASERO_RATE_TO_SPEED_CD(rate)          (guint) ((rate) / CD_RATE)
#define BRASERO_RATE_TO_SPEED_DVD(rate)         (guint) ((rate) / DVD_RATE)

In brasero-0.7.0/src/brasero-drive-properties.c:284:
In brasero-0.7.1/src/brasero-drive-properties.c:284:

        /* Speed combo */
        media = NCB_MEDIA_GET_STATUS (drive);
        max_rate = NCB_MEDIA_GET_MAX_WRITE_RATE (drive);
        if (media & BRASERO_MEDIUM_CD)
                step = CD_RATE;
        else
                step = DVD_RATE;

        model = gtk_combo_box_get_model (GTK_COMBO_BOX (priv->speed));

        max_text = g_strdup_printf (_("Max speed"));

        gtk_list_store_append (GTK_LIST_STORE (model), &iter);
        gtk_list_store_set (GTK_LIST_STORE (model), &iter,
                            PROP_TEXT, max_text,
                            PROP_RATE, max_rate,
                            -1);
        g_free (max_text);

        gtk_combo_box_set_active (GTK_COMBO_BOX (priv->speed), 0);

        for (rate = max_rate - step; rate > step; rate -= step * 2) {
                GtkTreeIter iter;

                text = brasero_drive_properties_format_disc_speed (media, rate);
                gtk_list_store_append (GTK_LIST_STORE (model), &iter);
                gtk_list_store_set (GTK_LIST_STORE (model), &iter,
                                    PROP_TEXT, text,
                                    PROP_RATE, rate,
                                    -1);
                g_free (text);

                /* we do this to round things and get the closest possible speed */
                if ((rate / step) == (default_rate / step))
                        gtk_combo_box_set_active_iter (GTK_COMBO_BOX (priv->speed), &iter);
        }

/*=====================================================================*/

How can you explain that in Brasero 0.6 you don't have to decrement the maximum speed, as it's already an even value:
for (i = max_speed; i > 0; i -= 2) {

...whereas in Brasero 0.7 the code toggles the odd/even:
for (rate = max_rate - step; rate > step; rate -= step * 2) {

Not to mention that I am unable to understand the following comment:
/* we do this to round things and get the closest possible speed */
                if ((rate / step) == (default_rate / step))

I'm out of neurones right now.

I see a big difference in the 0.6 algorithm: if you look at the SPEED_TO_RATE macros, they round the max_speed (by doing * 10.0 / 10.0)! So a 12.2x rate gets rounded down to 12, and so you get even speeds (the for loop doesn't start by decreasing the max_rate by a whole rate like 7.1 does). With 7.1, a 12.2x max rate will produce an odd-numbered list (max speed, then 12.2 - a whole rate is 11.2, int-devided by rate gives 11x, and so on).

I'll stop now and I'll let the upstream authors comment on this. Luis, Philippe ?

I forgot to comment on:

> This reports 4 available writeable speeds :
>  - 28.2 x
>  - 18.4 x
>  - 11.5 x
>  - 4.6 x

It is indeed why I previously had the maximum speed of 28x (not tested), however note that 18x (??? 20x ???) and ??? 5x ??? are not necessarily working... but 12x works well with my unit!

K3b only "sees" 24x, 16x and 10x on this burner, and I know that 10x is a working speed in my case.

Trying to burn at "11x" fails in practice.

It seems that one of the following is true:
(1) LG drives report wrong speed burning vales, but then how do you explain that Nero for Windows is guessing them correctly? (Not on my laptop, but I know it does!)
(2) We don't know how to read the speeds correctly. We don't know, K3b doesn't know (although it has sensible guesses), etc.

There is a need for a better documentation. I already don't know how to read the speeds in "00 00 10 8a 00 00 0b 06  00 00 06 e4 00 00 02 c2", but maybe there is some other "correction factor" that we should ask the drive for.

(In reply to comment #6)
> I see a big difference in the 0.6 algorithm: if you look at the SPEED_TO_RATE
> macros, they round the max_speed (by doing * 10.0 / 10.0)! 
> So a 12.2x rate gets  rounded down to 12, and so you get even speeds 
> (the for loop doesn't start by decreasing the max_rate by a whole rate 
> like 7.1 does). With 7.1, a 12.2x max rate will produce an odd-numbered
> list (max speed, then 12.2 - a whole rate is
> 11.2, int-devided by rate gives 11x, and so on).

So the 0.6 algorithm is better, why have they changed it??????????

In my CD case: 28.2x gets 28x, then it goes down from 28x to 2x -> OK.
In your DVD case: your speeds are exactly 4x and 2x -> OK.

With Brasero 0.7 though:

In my CD case: 28.2x gets 27x, then it goes down from 27x to 1x -> FAILURE.
In your DVD case: your speeds are exactly 3x and 1x -> FAILURE.

Why don't round in 0.7 too, then skip the bloody decrement by one? (skipping the decrement was what I suggested initially)

In my CD case: 28.2x gets 28x, then it goes down from 28x to 2x -> OK.
In your DVD case: your speeds are exactly 4x and 2x -> OK.

What I *don't* understand is this:
??? How is 0.7's algorithm giving correct *CD* speeds with *your* hardware???
??? How is 0.7's algorithm giving correct *DVD* speeds with *my* hardware???

O.6 mainly relied on nautilus-cd-burn (that is HAL in fact) for speed detection, the way it rounds speeds (seems) to be much better than mine. For many reasons I implemented all the SCSI functions and dropped nautilus-cd-burner (completely dropped in trunk now) hence the changes in displaying speed.

Now I chose to use this simplistic algorithm for various reasons:
- many users expect to see that sort of soft increment (at least those who don't know much about CD burning)
- that was also because I didn't know what to do with older drives that don't have GET_PERFORMANCE nor can return a speed list in mechanical page.

That said I agree with Denis the way I display speeds is simplistic and even lame. In the end the last argument doesn't stand since there are no longer many old drives; as for the first, well, I'll close them as they are filed and try to educate people. I checked and nautilus-cd-burner now uses the list of speeds returned by the drive so I'll do the same.

For all these reasons, I'll make brasero display the speeds returned by the drive, not even rounded that will prevent failures.
I expect a patch to be ready by tomorrow for trunk.

done in trunk. Now we display the speeds returned by drive instead of making up some with the increment. For me the speed (since there is only one returned) is still odd and isn't an integer. But that will probably fix a few problems.