Re: [spice-gtk v4 14/24] file-xfer: call progress_callback per FileTransferOperation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Wed, Jun 29, 2016 at 11:41:15AM -0500, Jonathon Jongsma wrote:
> On Tue, 2016-06-28 at 14:39 +0200, Victor Toso wrote:
> > I'm thinking about it more carefully now and I think I got what you
> > mean. It really depends on how read_bytes and transfer_size will be
> > used by the API user so, we can define the behavior that we want.
> >
> > To make both proposals clear, let's take an example of a transfer
> > operation of three files where one of them will be cancelled.

> >
> >  | File | Size  | Sent   | Pending |
> >  |  A   | 800kb | 100kb  |  700kb  |
> >  |  B   | 800kb | 400kb  |  400kb  |
> >  |  C   | 800kb | 600kb  |  200kb  |
> >  ----------------------------------
> >  Total:  2400kb | 1100kb | 1300kb |
> >
> > 1-) With proposal from this patch
> >
> >  | -------- | transfer_size | total_sent | percentage |
> >  |  Before  |    2400kb     |   1100kb   |   45.83 %  |
> >  | Cancel A |    1600kb     |   1000kb   |   62.50 %  |
> >  | Cancel B |    1600kb     |    700kb   |   43.75 %  |
>
> should be:         800kb ?         600kb ? |   75.00 % ?
>
> >  | Cancel C |    1600kb     |    500kb   |   31.50 %  |
>
> should be:            0kb ?   |      0 kb? |    0.00 % or 100.00 %

As A, B, C has the same file-size, with approach (1) the transfer_size
variable will be 1600kb considering that we are only cancelling one of
these tasks

>
> Or did I understand wrong?

Yep. My goal with the table was trying to show one cancellation but in
three different states { < 50%, 50%, > 50% } to evaluate the result with
each alternative

>
> >
> > What I like is that the percentage is 100% related to the actual read
> > and actual transfer size; What I don't like is that the percentage might
> > go up or down based on cancel/errors.
> >
> > 2-) With your proposal (task complete by changing transfer-size)
> >
> >  | -------- | transfer_size | total_sent | percentage |
> >  |  Before  |    2400kb     |   1100kb   |   45.83 %  |
> >  | Cancel A |    1700kb     |   1100kb   |   64.70 %  |
> >  | Cancel B |    2000kb     |   1100kb   |   55.75 %  |
>
> should be:         1300kb     |   1100kb   |   84.62 %  |
>
> >  | Cancel C |    2200kb     |   1100kb   |   50.00 %  |
>
> should be:         1100kb     |   1100kb   |  100.00 %
>
> Essentially, subtract the "Pending" value from transfer_size when a
> task is cancelled
>
> >
> > What I like is that the percentage always increase which would
> > really match the expected behavior of cancel/error of a task
> >
> > 3-) With your proposal but changing the total_sent instead
> >
> >  | -------- | transfer_size | total_sent | percentage |
> >  |  Before  |    2400kb     |   1100kb   |   45.83 %  |
> >  | Cancel A |    2400kb     |   1800kb   |   75.00 %  |
> >  | Cancel B |    2400kb     |   1500kb   |   62.50 %  |
>
> should be:                        2200kb   |   91.67 %
>
> >  | Cancel C |    2400kb     |   1300kb   |   54.16 %  |
>
> should be:                        2400kb   |  100.00 %
>
> >
> > As expected, it also always increase the percentage.
>
> Your numbers didn't really match this statement ;)

No? For each task in different states, the percentage after cancellation
will be higher then before (45.83%). The comment is that it should be
expected to be similar to your approach (2).

>
> >
> > If there is not real standard behind this and it is up to us to
> > decide, I would either stick with option (1) and in case of
> > cancel/error we are sending the correct value of pending files and
> > the user API could handle the progress data as it wishes; or I would
> > go for option (3) as it is simpler to say that "we add the pending
> > bytes to total_sent on cancel/error" but also, the percentage is
> > what you should expect in case that file was transferred completely
> > (75% if File A was transferred while B and C were in the 'before'
> > state)
> >
> > Let me know what you think :)
>
> well, I think approach 3 is a bit of a lie since it implies that we
> sent a lot more data than we really did. If a client wanted to use the
> amount of data sent to try to calculate and display a transfer rate in
> the UI, that would not work.

True.

> Similarly, you couldn't really use option 1 for this purpose since it
> implies that we sent less data than we actually did.

True.

> So I would personally still go for option 2.

Agreed :)

>
> >
> > >
> > >
> > >
> > >
> > > >
> > > >      /* Remove and free SpiceFileTransferTask */
> > > >      g_hash_table_remove(xfer_op->xfer_task_ht,
> > > > GUINT_TO_POINTER(task_id));
> > > >      g_object_unref(xfer_task);
> > > > @@ -3141,6 +3128,23 @@ static void
> > > > file_transfer_operation_task_finished(SpiceFileTransferTask *xfer_ta
> > > >          file_transfer_operation_end(xfer_op);
> > > >  }
> > > >  
> > > > +static void file_transfer_operation_send_progress(SpiceFileTransferTask
> > > > *xfer_task)
> > > > +{
> > > > +    FileTransferOperation *xfer_op;
> > > > +    SpiceMainChannel *channel;
> > > > +    guint32 task_id;
> > > > +
> > > > +    channel = spice_file_transfer_task_get_channel(xfer_task);
> > > > +    task_id = spice_file_transfer_task_get_id(xfer_task);
> > > > +    xfer_op = g_hash_table_lookup(channel->priv->file_xfer_tasks,
> > > > GUINT_TO_POINTER(task_id));
> > > > +    g_return_if_fail(xfer_op != NULL);
> > > > +
> > > > +    if (xfer_op->progress_callback)
> > > > +        xfer_op->progress_callback(xfer_op->stats.total_sent,
> > > > +                                   xfer_op->stats.transfer_size,
> > > > +                                   xfer_op->progress_callback_data);
> > > > +}
> > > > +
> > > This strikes me as a little bit convoluted. The function is ostensibly
> > > a FileTransferOperation method (by the name and by the behavior), yet
> > > the function argument is SpiceFileTransferTask*. And to get from
> > > SpiceFileTransferTask to FileTransferOperation, we have to first get
> > > the SpiceMainChannel and ID from the task, then look up the operation
> > > from a hash table in the channel. This is one of the reasons that I
> > > thought it might be better for the task structure to have a reference
> > > to its operation rather than storing the operation in a hash table in
> > > the channel indexed on task ID. But perhaps my solution won't work out
> > > very well in real life, and I may just being overly nitpick-y here.
> > > Feel free to tell me so if I am ;)
> > Not at all, I agree with you. Still, it is a bit hard to make everything
> > play nice in one batch! In my opinion, the file_xfer_* functions were
> > not meant to work per file-transfer-operation but per-file-transfer and
> > so we get this problem above.
> > 
> > In this case, this function is called when data is flushed at
> > file_xfer_data_flushed_cb and I really think we should have a xfer_op
> > pointer.
> > 
> > I think we can better organize this if we use SpiceFileTransferTask as
> > source_object and pass FileTransferOperations as user_data of this
> > read_async/flush functions.
> > 
> > I would prefer to address this later if you don't feel it is a blocker,
> > too many patches already :)
> 
> Yes, we can address it later.
> 
> 
> > 
> > > 
> > > 
> > > > 
> > > >  static SpiceFileTransferTask
> > > > *spice_file_transfer_task_new(SpiceMainChannel
> > > > *channel,
> > > >                                                             GFile *file,
> > > >                                                             GCancellable
> > > > *cancellable);
> > > > @@ -3210,12 +3214,12 @@ void spice_main_file_copy_async(SpiceMainChannel
> > > > *channel,
> > > >  
> > > >      xfer_op = g_new0(FileTransferOperation, 1);
> > > >      xfer_op->channel = channel;
> > > > +    xfer_op->progress_callback = progress_callback;
> > > > +    xfer_op->progress_callback_data = progress_callback_data;
> > > >      xfer_op->xfer_task_ht =
> > > > spice_file_transfer_task_create_tasks(sources,
> > > >                                                                    channel
> > > > ,
> > > >                                                                    flags,
> > > >                                                                    cancell
> > > > able
> > > > ,
> > > > -                                                                  progres
> > > > s_ca
> > > > llback,
> > > > -                                                                  progres
> > > > s_ca
> > > > llback_data,
> > > >                                                                    callbac
> > > > k,
> > > >                                                                    user_da
> > > > ta);
> > > >      g_hash_table_iter_init(&iter, xfer_op->xfer_task_ht);
> > > > @@ -3234,7 +3238,7 @@ void spice_main_file_copy_async(SpiceMainChannel
> > > > *channel,
> > > >  
> > > >          spice_file_transfer_task_init_task_async(xfer_task,
> > > >                                                   file_xfer_init_task_asyn
> > > > c_cb
> > > > ,
> > > > -                                                 NULL);
> > > > +                                                 xfer_op);
> > > >      }
> > > >  }
> > > >  
> > > > @@ -3279,6 +3283,18 @@ static GCancellable
> > > > *spice_file_transfer_task_get_cancellable(SpiceFileTransferT
> > > >      return self->cancellable;
> > > >  }
> > > >  
> > > > +static guint64
> > > > spice_file_transfer_task_get_file_size(SpiceFileTransferTask
> > > > *self)
> > > > +{
> > > > +    g_return_val_if_fail(self != NULL, 0);
> > > > +    return self->file_size;
> > > > +}
> > > > +
> > > > +static guint64
> > > > spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask
> > > > *self)
> > > > +{
> > > > +    g_return_val_if_fail(self != NULL, 0);
> > > > +    return self->read_bytes;
> > > > +}
> > > > +
> > > >  /* Helper function which only creates a SpiceFileTransferTask per GFile
> > > >   * in @files and returns a HashTable mapping task-id to the task itself
> > > >   * Note that the HashTable does not free its values uppon destruction:
> > > > @@ -3288,8 +3304,6 @@ static GHashTable
> > > > *spice_file_transfer_task_create_tasks(GFile **files,
> > > >                                                           SpiceMainChannel
> > > > *channel,
> > > >                                                           GFileCopyFlags
> > > > flags,
> > > >                                                           GCancellable
> > > > *cancellable,
> > > > -                                                         GFileProgressCal
> > > > lbac
> > > > k progress_callback,
> > > > -                                                         gpointer
> > > > progress_callback_data,
> > > >                                                           GAsyncReadyCallb
> > > > ack
> > > > callback,
> > > >                                                           gpointer
> > > > user_data)
> > > >  {
> > > > @@ -3312,8 +3326,6 @@ static GHashTable
> > > > *spice_file_transfer_task_create_tasks(GFile **files,
> > > >  
> > > >          xfer_task = spice_file_transfer_task_new(channel, files[i],
> > > > task_cancellable);
> > > >          xfer_task->flags = flags;
> > > > -        xfer_task->progress_callback = progress_callback;
> > > > -        xfer_task->progress_callback_data = progress_callback_data;
> > > >          xfer_task->callback = callback;
> > > >          xfer_task->user_data = user_data;
> > > >  
> > > looks good other than discussion points above
> > > 
> > > Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > As always, many thanks!
> >   toso
> > 
> > > 
> > > 
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]