Resync ffvpx decoder to FFmpeg 3.4

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

2 years ago
FFmpeg current release is 3.3.
Assignee

Comment 1

2 years ago
FFmpeg 3.4 has multi-threaded slice decoding which would greatly benefit the use with WebRTC
Assignee: nobody → jyavenard
Summary: Resync ffvpx decoder to FFmpeg 3.3 one → Resync ffvpx decoder to FFmpeg 3.4
Assignee

Comment 2

2 years ago
crashetest in bug 1180881 causes the updated ffvp8 decoder to crash.
See Also: → 1180881
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8921993 [details]
Bug 1366201 - P1. Revert "Bug 1274256 - TSan: data race gfx/layers/client/TextureClient.cpp:1161 in mozilla::layers::MappedYCbCrChannelData::CopyInto."

https://reviewboard.mozilla.org/r/192974/#review198356

r+, trusting that it's just a revert.
But do you really need to revert this? ('volatile' feels worse than 'atomic_int')

::: media/ffvpx/libavcodec/pthread_frame.c:474
(Diff revision 1)
>  }
>  
>  void ff_thread_report_progress(ThreadFrame *f, int n, int field)
>  {
>      PerThreadContext *p;
> -    atomic_int *progress = f->progress ? (atomic_int*)f->progress->data : NULL;
> +    volatile int *progress = f->progress ? (int*)f->progress->data : NULL;

'volatile' is almost certainly incorrect if it's used for thread safety (compared to atomic_int), it would be interesting to see why they use it.
Attachment #8921993 - Flags: review?(gsquelart) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8921994 [details]
Bug 1366201 - P2. Revert "Bug 1292039: [ffmpeg] P1. Remove unused options and dictionary features."

https://reviewboard.mozilla.org/r/192976/#review198358

r+ trusting it's only a revert.
Attachment #8921994 - Flags: review?(gsquelart) → review+
Assignee

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8921993 [details]
Bug 1366201 - P1. Revert "Bug 1274256 - TSan: data race gfx/layers/client/TextureClient.cpp:1161 in mozilla::layers::MappedYCbCrChannelData::CopyInto."

https://reviewboard.mozilla.org/r/192974/#review198356

> 'volatile' is almost certainly incorrect if it's used for thread safety (compared to atomic_int), it would be interesting to see why they use it.

this is the original code before our fix. FFmpeg developed their own fix, and is included in P3.
it ises stdatomics instead.
Assignee

Comment 12

2 years ago
(In reply to Gerald Squelart [:gerald] from comment #9)

> 'volatile' is almost certainly incorrect if it's used for thread safety
> (compared to atomic_int), it would be interesting to see why they use it.

They don't use it for thread safety. This revert out us back to what FFMPEG 3.2 code was using. In P3 we include FFMPEG official change instead.
https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/pthread_frame.c#L560

Comment 13

2 years ago
mozreview-review
Comment on attachment 8921995 [details]
Bug 1366201 - P3. Update ffvp9/ffvp8 to 3.4 branch.

https://reviewboard.mozilla.org/r/192978/#review198364

r+ trusting the outside update, I cannot review the whole thing!
Attachment #8921995 - Flags: review?(gsquelart) → review+

Comment 14

2 years ago
mozreview-review
Comment on attachment 8921996 [details]
Bug 1366201 - P4. Remove no longer necessary files.

https://reviewboard.mozilla.org/r/192980/#review198366
Attachment #8921996 - Flags: review?(gsquelart) → review+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8921966 [details]
Bug 1366201 - P5. Get around FFmpeg bug with corrupted data.

https://reviewboard.mozilla.org/r/192968/#review198370
Attachment #8921966 - Flags: review?(gsquelart) → review+

Comment 16

2 years ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6becd67f09e1
P1. Revert "Bug 1274256 - TSan: data race gfx/layers/client/TextureClient.cpp:1161 in mozilla::layers::MappedYCbCrChannelData::CopyInto." r=gerald
https://hg.mozilla.org/integration/autoland/rev/928c8f13b896
P2. Revert "Bug 1292039: [ffmpeg] P1. Remove unused options and dictionary features." r=gerald
https://hg.mozilla.org/integration/autoland/rev/d334969fa9f6
P3. Update ffvp9/ffvp8 to 3.4 branch. r=gerald
https://hg.mozilla.org/integration/autoland/rev/4e3060390bcd
P4. Remove no longer necessary files. r=gerald
https://hg.mozilla.org/integration/autoland/rev/6a0dc4602c74
P5. Get around FFmpeg bug with corrupted data. r=gerald
Assignee

Updated

2 years ago
Blocks: 1411883
Assignee

Updated

2 years ago
Depends on: 1414596
You need to log in before you can comment on or make changes to this bug.