On Mon, Oct 17, 2016 at 1:30 PM, Diego Biurrun <di...@biurrun.de> wrote:
> Looking at this in more detail now that the first round of review is over.
>
> On Mon, Oct 17, 2016 at 11:54:04AM -0400, Vittorio Giovara wrote:
>> --- a/tests/fate-run.sh
>> +++ b/tests/fate-run.sh
>> @@ -76,6 +76,11 @@ probefmt(){
>>
>> +probear(){
>> + run avprobe -show_stream_entry sample_aspect_ratio -v 0 "$@"
>> + run avprobe -show_stream_entry display_aspect_ratio -v 0 "$@"
>> +}
>> +
>> --- a/tests/fate/probe.mak
>> +++ b/tests/fate/probe.mak
>> @@ -16,3 +16,16 @@ fate-probe-format: $(FATE_PROBE_FORMAT)
>> $(FATE_PROBE_FORMAT): avprobe$(EXESUF)
>> +$(FATE_MOV): avprobe$(EXESUF)
>
> You are duplicating the dependency list, don't.
How do you mean? Is it enough to do
$(FATE_PROBE_FORMAT) += $(FATE _MOV)
>> +FATE_MOV += fate-mov-display-matrix
>> +fate-mov-display-matrix: CMD = run avprobe -v 0 -show_stream_entry matrix
>> $(TARGET_SAMPLES)/mov/displaymatrix.mov
>> +
>> +FATE_MOV += fate-mov-rotation
>> +fate-mov-rotation: CMD = run avprobe -v 0 -show_stream_entry rotation
>> $(TARGET_SAMPLES)/mov/displaymatrix.mov
>> +
>> +FATE_MOV += fate-mov-ar
>> +fate-mov-ar: CMD = probear $(TARGET_SAMPLES)/mov/displaymatrix.mov
>
> This is inconsistent. You added a convenience function for the last
> test, but not for the first two. You could simply run the last test
> "manually" and then separate the tests for sar and dar - and separating
> tests is always a good thing IMO. Or you could add a convenience
> function that can be used in all tests.
I know it's inconsistent, but I moved the AR tests in a separate
function because I thought it might usable in other tests, while
rotation and displaymatrix are something that are meaningful only for
mov.
--
Vittorio
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
