Spanification of jpeg_parser and related files [chromium/src : main]
Sergio Solano (Gerrit)
Sergio Solano abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 9 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Auto-Submit | +1 |
9 comments
Stephen NuskoShould this have been removed as part of https://crbug.com/450466845 ?
Colin BlundellDo you know who we should ask on that front? Is it Colin?
Colin BlundellI had thought that it was still used via `VaapiMjpegDecodeAccelerator` when examining it as part of that workstream. However, looking more closely it looks like that might just be a [dead include](https://chromium-review.googlesource.com/c/chromium/src/+/7228707). If that CL passes trybots I'll do a followup one removing the files.
Sergio SolanoAh no, my earlier finding was correct: it's used [here](https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/vaapi/vaapi_mjpeg_decode_accelerator.cc;l=139?q=vaapi_mjpeg_&ss=chromium), and that class is itself used by //components/chromeos_camera.
Acknowledged
auto pic_param_components = base::span(pic_param->components);
auto frame_header_components = base::span(frame_header.components);Sergio Solanoshould we `.first(frame_header.num_components)`?
Obviously this is fine since it is getting the size compile time and doing the bounds checks based on that, but might be a bit clearer.
Done
auto load_quantiser_table_span = base::span(iq_matrix->load_quantiser_table);Stephen NuskoI don't know if you need the span?
Sergio Solanosorry what I meant by this is the `_span` in the variable name. Obviously need the span to remove the UNSAFE_TODO.
Done
dest_span.copy_from(src_span);Sergio Solanonit: copy_from_nonoverlapping
Why? They are clearly different tables so nonoverlapping, this is more performant.
Also please take a look at other locations.
Done
const auto& dc_src_table = dc_table[i];
const auto& ac_src_table = ac_table[i];Sergio SolanoI don't think these temporaries make the code read better? Perhaps just leave them as dc_table[i]?
Done
.first(num_dc_values)
.copy_from(base::span(dc_src_table.code_value).first(num_dc_values));Sergio Solano`copy_prefix_from` would save you one of these `.first`
Done
uint8_t code_value[162];Sergio Solanonit: switch this one while we are here just to be consistent in the same type?
Done
auto code_length_span = base::span(table->code_length);Sergio SolanoNo need for the temporary code_length_span variable, just inline it into the for loop.
Done
ptrdiff_t span_size = eoi_begin_ptr - base::as_chars(result->data).data();Sergio Solanonit: variable name
perhaps
start_of_eoi_data?
what about `scan_data_size`?
Related details
- Colin Blundell
- Dale Curtis
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |